Initial support for kernel 6.18 on Ayn odin2#9286
Conversation
github-actions
bot
added
02
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughUpgrade SM8550 edge kernel to 6.18 and add 18 kernel patches: many new drivers, DT nodes, MMC/clock fixes, UFS/USB tweaks, serial-derived Wi‑Fi/Bluetooth address generation, and assorted subsystem adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant socinfo as socinfo.c
participant bt as drivers/bluetooth/btqca
participant ath as drivers/net/wireless/ath/ath12k
rect rgba(200,220,255,0.5)
socinfo->>socinfo: probe() read serial_number
socinfo-->>bt: export qcom_serial_number
socinfo-->>ath: export qcom_serial_number
end
rect rgba(200,255,200,0.5)
bt->>bt: on bt init read qcom_serial_number
bt->>bt: generate_bdaddr_from_serial()
bt->>bt: set hdev->public_addr / bdaddr
end
rect rgba(255,220,200,0.5)
ath->>ath: on mac init read qcom_serial_number
ath->>ath: generate_macaddr_from_serial()
ath->>ath: reverse bytes (ath12k_reverse_mac)
ath->>kernel: apply generated MAC during mac_hw_register
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 281-311: The RX path in rsinput_rx currently treats each serdev
callback as a full frame (copying into rx_buf, checking checksum, and dropping
on failure), which loses fragmented UART packets and causes spurious MCU resets;
change rsinput_rx to maintain a persistent per-device receive buffer (move
rx_buf to persist across callbacks inside struct rsinput_driver if not already),
append incoming data to that buffer instead of overwriting, implement a search
loop that locates frame headers and extracts complete frames (using
MCU_PKT_SIZE_MIN and frame length fields) before calling compute_checksum and
rsinput_process_data, and resynchronize by discarding bytes until a valid header
is found on checksum failures rather than reinitializing MCU immediately; also
remove the static prev_states in handle_cmd_status and make prev_states a member
of rsinput_driver so state is not shared across device instances.
- Around line 306-309: The receive_buf callback must not sleep; replace the
direct call to rsinput_init_commands(drv) in the branch where computed_checksum
!= received_checksum with scheduling a worker that runs rsinput_init_commands in
process context: add a struct work_struct reinit_work to struct rsinput_driver,
implement a static void rsinput_reinit_work(struct work_struct *work) that
container_ofs to struct rsinput_driver and calls rsinput_init_commands(drv),
initialize the work in probe with INIT_WORK(&drv->reinit_work,
rsinput_reinit_work), and in the receive_buf path call
schedule_work(&drv->reinit_work) (then keep the existing goto error flow).
- Around line 333-352: The driver stores potential error pointers in
drv->boot_gpio, drv->enable_gpio and drv->reset_gpio via devm_gpiod_get_optional
but only logs warnings, so later checks like if (drv->boot_gpio) can treat error
pointers as valid and gpiod_set_value_cansleep() will crash; change each of
those three acquisition blocks to return on error using
dev_err_probe(&serdev->dev, error, "Unable to get %s gpio\n", "<name>") (or
equivalent dev_err handling) so that if PTR_ERR(drv->*_gpio) indicates failure
you propagate the error (handling -EPROBE_DEFER properly) instead of continuing
and dereferencing the error pointer; apply the same pattern for boot, enable and
reset GPIO acquisitions referenced by devm_gpiod_get_optional and calls to
gpiod_set_value_cansleep().
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 286-301: The error paths after calling
mipi_dsi_picture_parameter_set(...) and mipi_dsi_compression_mode(...) currently
return without undoing power/reset state; modify those branches to perform the
same cleanup sequence used on successful shutdown/unprepare (disable regulators
and assert reset) before returning: invoke the existing power-off/unprepare
helper used elsewhere in this driver (e.g., the function that disables
regulators and asserts the panel reset — match the symbol used in this driver
such as icna3512_power_off or chipone_icna3512_unprepare) so both failure
branches cleanly disable supplies and assert reset, then return the error code.
In
`@patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Around line 282-284: The devm_gpiod_get() return for priv->sdb is only checked
against -EPROBE_DEFER which leaves other ERR_PTR values in priv->sdb and can
cause gpiod_set_value_cansleep() to oops; change the handling in the probe path
that calls devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH) to check
IS_ERR(priv->sdb): if PTR_ERR(priv->sdb) == -EPROBE_DEFER return -EPROBE_DEFER,
otherwise log an error (including PTR_ERR(priv->sdb) and context) and return the
error code (PTR_ERR(priv->sdb)) so no invalid ERR_PTR is used later by
gpiod_set_value_cansleep() or related calls.
- Around line 292-309: After enabling the regulator with
regulator_enable(priv->vdd) you must disable it on subsequent probe failures to
avoid leaking the regulator; change the probe flow so failures in
htr3212_init_regs() or htr3212_parse_dt() jump to a cleanup label (e.g.
err_disable_reg) that calls regulator_disable(priv->vdd) before returning, or
explicitly call regulator_disable(priv->vdd) in those error return paths; update
the probe function around regulator_enable, htr3212_init_regs, and
htr3212_parse_dt to use this cleanup pattern (and remove early returns that
bypass regulator_disable).
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 43-48: The PRIMARY_MI2S_RX branch currently calls
clk_prepare_enable(pdata->i2s_clk) and two snd_soc_dai_set_fmt() calls without
checking return values; add an int ret; at the top of the function, capture and
check ret = clk_prepare_enable(pdata->i2s_clk) and if negative log/return the
error, then call ret = snd_soc_dai_set_fmt(cpu_dai, fmt) and check/handle error
(undo clk with clk_disable_unprepare(pdata->i2s_clk) if needed) before calling
ret = snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt) and handle that error
similarly so failures are propagated and resources cleaned up.
In
`@patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch`:
- Around line 44-56: The OPP table used by sdhci on level‑shifter devices lacks
a 37.5 MHz entry so dev_pm_opp_set_rate(desired_rate) will fail when
msm_host->uses_level_shifter forces desired_rate to
LEVEL_SHIFTER_HIGH_SPEED_FREQ; add a new opp entry for 37.5 MHz to the
sdhc2_opp_table (the table added/modified in the patch that defines
qcom,use-level-shifter) including the correct opp-hz for 37,500,000 Hz and
sensible opp-peak-kBps / opp-avg-kBps values so
dev_pm_opp_set_rate(mmc_dev(host->mmc), LEVEL_SHIFTER_HIGH_SPEED_FREQ) succeeds
for devices that set qcom,use-level-shifter.
In `@patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch`:
- Around line 19-30: The current change removes the CNR readiness handshake
(handshake(..., XHCI_STS_CNR, ...)) and global dev_warn fallback, which can
break non-Odin2 controllers; restore the handshake and warning for all
controllers except the Odin2/SM8550 quirk: detect the affected HW by checking
the PCI device identity or an existing quirk flag on pdev (e.g., match PCI ID or
set/inspect a quirk in probe), and only skip the handshake and dev_warn when
that quirk/PCI ID is present; keep using op_reg_base, XHCI_STS_OFFSET,
XHCI_STS_CNR, the timeout variable, and dev_warn(&pdev->dev, ...) for the normal
path so other controllers retain the original behavior and diagnostics.
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Around line 108-141: The patch removes the previous NVM/firmware fallback and
hard-fails when generate_bdaddr_from_serial or qca_set_bdaddr fail; restore a
fallback to the original qca_check_bdaddr path instead of returning immediately
so BT can still use NVM BD_ADDR. Change qca_uart_setup to: call
generate_bdaddr_from_serial and, on error, log a warning and call
qca_check_bdaddr(&config) as the fallback; if generate succeeds but
qca_set_bdaddr fails, log the failure and fall back to qca_check_bdaddr(&config)
as well; only return an error if both the serial-derived path and
qca_check_bdaddr both fail. Ensure you still update
hdev->public_addr/hdev->bdaddr only on successful qca_set_bdaddr, and keep the
bt_dev_info log when the BD_ADDR is successfully set.
- Around line 245-268: generate_macaddr_from_serial is being used but its
success path and the per‑pdev uniqueness logic are inverted and missing offset:
if generate_macaddr_from_serial succeeds you must use the generated_macaddr
(call ath12k_reverse_mac into ar->mac_addr, log via ath12k_info) and add
ar->pdev_idx to the appropriate byte of generated_macaddr to avoid duplicate
MACs across pdevs; if generate_macaddr_from_serial fails, fall back to the
existing behavior that prefers ar->ab->pdevs_macaddr_valid and copies
pdev->mac_addr via ether_addr_copy, otherwise copy ar->ab->mac_addr and add
ar->pdev_idx as the current fallback does. Ensure you reference
generate_macaddr_from_serial, generated_macaddr, ath12k_reverse_mac,
ath12k_info, ether_addr_copy, ar->ab->pdevs_macaddr_valid and ar->pdev_idx when
making the change.
- Around line 20-90: Remove the unused static_bdaddr declaration and add the
missing include for <linux/ctype.h> in the source that defines
generate_bdaddr_from_serial (place the include immediately after the existing
`#include` "btqca.h") so isxdigit() resolves, and make the same include addition
in the other source that calls isxdigit (the ath12k/mac.c usage); ensure you
only remove the static const bdaddr_t static_bdaddr = { ... } symbol and do not
change the generate_bdaddr_from_serial logic or its bt_dev_err/bt_dev_info
calls.
In
`@patch/kernel/archive/sm8550-6.18/0017-Disable-sdhci_pltfm_free-calling.patch`:
- Around line 15-21: The sdhci_pltfm_free(pdev) call was commented out, causing
resource leaks; restore cleanup but guard it so it only runs when platform data
was actually initialized to avoid double-free. In sdhci_msm_probe and the
corresponding sdhci_msm_remove, add a boolean or check a pointer/flag stored on
the platform_device (e.g., msm_host or a ->pdata_initialized flag) when
allocation/setup succeeds, set it to true, and only call sdhci_pltfm_free(pdev)
when that flag/pointer indicates initialization; ensure the flag is cleared
before early returns and on remove to keep teardown symmetric and safe.
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1580-1694: The firmware paths in the odin2 DTS overlay are
incorrectly pointing to "qcom/sm8550/ayn/odin2portal/"; update the six
firmware-name properties to the correct "qcom/sm8550/ayn/odin2/" path (change
entries in the &gpu zap-shader node, &remoteproc_adsp node, &remoteproc_cdsp
node, and both &spk_amp_l and &spk_amp_r nodes), or alternatively refactor to a
shared firmware path/label and reference that from both odin2 and odin2portal so
the Device Tree doesn't mix Portal-specific paths into the base odin2 device.
🟡 Minor comments (7)
patch/kernel/archive/sm8550-6.18/0019-HACK-Revert-back-to-6.12-aw88166-driver.patch-36-39 (1)
36-39: Typo in error message: "32r-bit" should be "32-bit".Proposed fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch-218-223 (1)
218-223: Verify DRM panel callback assignment:.disablevs.unprepare.The
td4328_unpreparefunction (which performs power-off with regulator disable) is assigned to.disableinstead of the semantically correct.unpreparecallback. The struct leaves.unprepareas NULL. While the functions themselves are correctly implemented for power sequencing, this pairing is unconventional—the DRM framework typically expects.prepare/.unpreparefor power control. The code is auto-generated and marked WIP, but the assignment should use.unpreparefor semantic correctness and to ensure the framework invokes the power-down sequence as designed.Suggested fix
static const struct drm_panel_funcs td4328_panel_funcs = { .prepare = td4328_prepare, - .disable = td4328_unprepare, + .unprepare = td4328_unprepare, .get_modes = td4328_get_modes, .get_orientation = td4328_get_orientation, };Confirm this pattern works as intended on actual hardware, particularly during suspend/resume cycles.
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch-17-28 (1)
17-28: Missing Makefile entry for odin2portal DTB.The patch creates
qcs8550-ayn-odin2portal.dts(line 15) but only addsqcs8550-ayn-odin2.dtbto the Makefile. The Portal variant won't be built.Proposed fix
dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8550-ayn-odin2.dtb +dtb-$(CONFIG_ARCH_QCOM) += qcs8550-ayn-odin2portal.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtbpatch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-348-348 (1)
348-348: Missing closing angle bracket in MODULE_AUTHOR.Typo:
<teguh@sobir.in"should be<teguh@sobir.in>".Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch-197-198 (1)
197-198: Readalways-onproperty from the child LED node, not parent device
device_property_read_bool(dev, "always-on")reads from the parent device node, but this function parses individual LED child nodes. Thealways-onproperty should be per-LED and read from thechildnode instead. Other per-LED properties likeregare correctly read fromchildon line 137.Proposed fix
- if (!device_property_read_bool(dev, "always-on")) + if (!device_property_read_bool(&of_node_to_device(child), "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;Or more idiomatically:
- if (!device_property_read_bool(dev, "always-on")) + if (!of_property_read_bool(child, "always-on")) cdev->flags |= LED_CORE_SUSPENDRESUME;patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch-22-24 (1)
22-24: Tighten the Kconfig prompt wording.The prompt is user‑facing; tweak grammar for clarity.
💬 Suggested wording
-config JOYSTICK_RSINPUT - tristate "UART Based gamepad driver that found in AYN and Retroid Pocket products" +config JOYSTICK_RSINPUT + tristate "UART-based gamepad driver found in AYN and Retroid Pocket products"patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch-210-237 (1)
210-237: Make button state per‑device instead of static.
static unsigned long prev_statesis shared across all devices; multiple RSInput pads will corrupt each other’s state. Store it instruct rsinput_driver.🧩 Per-device state
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_states; }; - static unsigned long prev_states; - unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); + unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); unsigned long current_states = keys, changes; @@ - bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); + bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap)); @@ - prev_states = keys; + drv->prev_states = keys;
🧹 Nitpick comments (12)
patch/kernel/archive/sm8550-6.18/0019-HACK-Revert-back-to-6.12-aw88166-driver.patch (1)
370-374: Minor style: Missing space before{.Linux kernel coding style requires a space before the opening brace.
Proposed fix
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { dev_dbg(dai->dev, "STREAM_PLAYBACK stop");patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch (3)
143-176: Kernel coding style violations in init sequence.Several issues that would likely be flagged by checkpatch:
- Line 163:
else {should be on same line as closing brace:} else {- Mixed indentation throughout - kernel style requires tabs, but many lines use spaces (e.g., lines 152, 155-161, 164-175, 178-182, 190-192)
- Line 145: Unnecessary blank line after opening brace
Example fix for else placement
- } - else { + } else {
432-443: Initial backlight brightness set to maximum.The backlight is initialized with
brightness = max_brightness = 4096. This means the display will be at full brightness on boot. If this is intentional for the Odin2 device, it's fine, but typically panels start at a lower default.
489-501: Coding style issues in probe function.
- Line 497: Missing space before brace:
if (ret < 0){should beif (ret < 0) {- Lines 491-494, 496, 500: Use spaces instead of tabs for indentation
Style fix
- ret = mipi_dsi_attach(pinfo->dsi); - if (ret < 0){ + ret = mipi_dsi_attach(pinfo->dsi); + if (ret < 0) { dev_err_probe(dev, ret, "Failed to attach to DSI host\n"); drm_panel_remove(&pinfo->panel); - return ret; - } + return ret; + }patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch (1)
107-109: Consider checking return values for vendor-specific DSI commands.The
mipi_dsi_dcs_write_seqcalls don't check return values. While not critical for auto-generated panel drivers, checking could help diagnose initialization issues.patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (3)
244-358: Commented-out code in thermal zone definitions.Multiple trip points and cooling maps are commented out (lines 247-263, 307-317, 349-356). If these are unused, consider removing them to keep the DT clean. If they're reference values for future tuning, a brief comment explaining their purpose would help maintainability.
1696-1704: Inconsistent indentation in soc node.Lines 1697-1703 use 4 spaces instead of tabs for indentation, which is inconsistent with the rest of the device tree. Similarly, line 1574 (
backlight = <&backlight>;) appears to have space indentation.Proposed fix for soc node
&soc { - qcom_tzlog: qcom_tzlog { - status = "disabled"; - }; + qcom_tzlog: qcom_tzlog { + status = "disabled"; + }; - arch_timer: arch_timer { - status = "disabled"; - }; + arch_timer: arch_timer { + status = "disabled"; + }; };
1862-1876: Potentially confusing LED node naming convention.The node name
ledl_b1suggests "blue" but hascolor = <LED_COLOR_ID_RED>andlabel = "l:r1". This pattern continues withledl_g1having green andledl_r1having blue. If the node names follow a hardware register mapping rather than color semantics, consider adding a brief comment to prevent confusion during maintenance.patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (2)
22-30: UnnecessaryREGMAP_I2Cselection.The Kconfig selects
REGMAP_I2C, but the driver usesi2c_smbus_write_byte_data()directly instead of the regmap API. Consider removing theselect REGMAP_I2Cline, or refactoring the driver to use regmap for consistency.
132-141: Minor simplification possible.The explicit
return 0after the error check is redundant sinceretwould be 0 on success.Optional simplification
static int htr3212_reset_regs(struct htr3212_priv *priv) { - int ret; - - ret = htr3212_write(priv, HTR3212_RESET, 0x00); - if (ret) - return ret; - - return 0; + return htr3212_write(priv, HTR3212_RESET, 0x00); }patch/kernel/archive/sm8550-6.18/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch (1)
31-33: Makeqcom,use-level-shifterexplicitly boolean in the schema.Without a type, dt-schema will accept non-boolean values. Tightening the schema avoids invalid DTS values slipping through.
♻️ Proposed schema tightening
qcom,use-level-shifter: + type: boolean description: Flag to indicate if platform has level shifter for SD card.patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
153-163: Remove unused static_macaddr
static_macaddrisn’t referenced and may trigger unused‑const warnings under W=1. Drop it or wire it as a fallback default.🧹 Remove unused static_macaddr
-/* Define a static, predefined MAC_ADDR structure */ -static const struct macaddr_t static_macaddr = { - .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } -};
| +static size_t rsinput_rx(struct serdev_device *serdev, const u8 *data, size_t count) { | ||
| + struct rsinput_driver *drv = serdev_device_get_drvdata(serdev); | ||
| + uint8_t received_checksum, computed_checksum; | ||
| + | ||
| + if (!drv || !data || count == 0) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Invalid RX data\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + if (count > sizeof(drv->rx_buf)) { | ||
| + dev_warn_ratelimited(&serdev->dev, "RX buffer overflow\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + memcpy(drv->rx_buf, data, count); | ||
| + | ||
| + if (count < MCU_PKT_SIZE_MIN) { | ||
| + dev_warn_ratelimited(&serdev->dev, "Frame too short for checksum validation\n"); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + received_checksum = drv->rx_buf[count - 1]; | ||
| + | ||
| + computed_checksum = compute_checksum(drv->rx_buf, count); | ||
| + | ||
| + if (computed_checksum != received_checksum) { | ||
| + rsinput_init_commands(drv); | ||
| + goto error; | ||
| + } | ||
| + | ||
| + rsinput_process_data(drv, drv->rx_buf, count); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "*rsinput*" "*0002*" 2>/dev/null | head -20Repository: armbian/build
Length of output: 170
🏁 Script executed:
fd -type f "0002-input-Add-driver-for-RSInput-Gamepad.patch" 2>/dev/nullRepository: armbian/build
Length of output: 39
🏁 Script executed:
find . -type f -name "*0002*" 2>/dev/nullRepository: armbian/build
Length of output: 9545
🏁 Script executed:
cat ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 15253
RX path assumes full frames arrive in a single callback, causing data loss on fragmented UART packets.
UART commonly fragments data across multiple receive callbacks. Current logic directly copies each callback into rx_buf, validates it as a complete frame, and drops it on checksum failure—overwriting any partial frame on the next callback. This causes loss of fragmented packets and spurious MCU reinitializations.
Implement persistent frame buffering: accumulate incoming data in a ring or linear buffer, search for frame headers, and assemble complete frames before validation. Handle resynchronization when stream alignment is lost.
Additionally, prev_states in handle_cmd_status() (line 168) is declared static, causing state leakage across multiple device instances. Make it per-device by moving it to the rsinput_driver struct.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 281 - 311, The RX path in rsinput_rx currently treats each serdev
callback as a full frame (copying into rx_buf, checking checksum, and dropping
on failure), which loses fragmented UART packets and causes spurious MCU resets;
change rsinput_rx to maintain a persistent per-device receive buffer (move
rx_buf to persist across callbacks inside struct rsinput_driver if not already),
append incoming data to that buffer instead of overwriting, implement a search
loop that locates frame headers and extracts complete frames (using
MCU_PKT_SIZE_MIN and frame length fields) before calling compute_checksum and
rsinput_process_data, and resynchronize by discarding bytes until a valid header
is found on checksum failures rather than reinitializing MCU immediately; also
remove the static prev_states in handle_cmd_status and make prev_states a member
of rsinput_driver so state is not shared across device instances.
| + if (computed_checksum != received_checksum) { | ||
| + rsinput_init_commands(drv); | ||
| + goto error; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the patch file and examine its structure
find . -name "*RSInput*" -o -name "*0002-input*" | head -20Repository: armbian/build
Length of output: 450
🏁 Script executed:
# Let's search for the patch file in the archive directory
fd -t f "0002-input-Add-driver-for-RSInput" patch/Repository: armbian/build
Length of output: 137
🏁 Script executed:
# Look for the specific patch file
find . -path "*/patch/kernel/archive/sm8550-6.18/0002-input*" -type fRepository: armbian/build
Length of output: 139
🏁 Script executed:
# List files in that directory
ls -la patch/kernel/archive/sm8550-6.18/ 2>/dev/null | head -20Repository: armbian/build
Length of output: 1967
🏁 Script executed:
cat -n patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | head -350 | tail -80Repository: armbian/build
Length of output: 2911
🏁 Script executed:
# Get the full patch to understand the context better
wc -l patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 141
🏁 Script executed:
# Let's see the area around lines 306-309 with more context
sed -n '290,330p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 1102
🏁 Script executed:
# Search for rsinput_rx function definition
sed -n '1,400p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | grep -n "rsinput_rx\|receive_buf"Repository: armbian/build
Length of output: 246
🏁 Script executed:
# Get broader context of the receive callback function
sed -n '280,350p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 2164
🏁 Script executed:
# Search for rsinput_init_commands implementation in the patch
sed -n '1,467p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | grep -n "rsinput_init_commands"Repository: armbian/build
Length of output: 210
🏁 Script executed:
# Get the full implementation of rsinput_init_commands
sed -n '1,467p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | sed -n '/^+static.*rsinput_init_commands/,/^+^}/p' | head -50Repository: armbian/build
Length of output: 1981
🏁 Script executed:
# Search more broadly for the function
grep -n "rsinput_init_commands" patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 210
🏁 Script executed:
# Get the function definition around those line numbers
sed -n '100,200p' patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patchRepository: armbian/build
Length of output: 3303
Don't sleep inside the serdev receive callback.
rsinput_init_commands() contains msleep(100) calls (at lines 160 and 168) and should not run in receive_buf context. Defer re‑init to a workqueue or threaded context.
Suggested fix
- if (computed_checksum != received_checksum) {
- rsinput_init_commands(drv);
- goto error;
- }
+ if (computed_checksum != received_checksum) {
+ schedule_work(&drv->reinit_work);
+ goto error;
+ }Additional changes needed:
/* in struct rsinput_driver */
struct work_struct reinit_work;
/* in probe */
INIT_WORK(&drv->reinit_work, rsinput_reinit_work);
/* worker */
static void rsinput_reinit_work(struct work_struct *work)
{
struct rsinput_driver *drv = container_of(work, struct rsinput_driver, reinit_work);
rsinput_init_commands(drv);
}🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 306 - 309, The receive_buf callback must not sleep; replace the
direct call to rsinput_init_commands(drv) in the branch where computed_checksum
!= received_checksum with scheduling a worker that runs rsinput_init_commands in
process context: add a struct work_struct reinit_work to struct rsinput_driver,
implement a static void rsinput_reinit_work(struct work_struct *work) that
container_ofs to struct rsinput_driver and calls rsinput_init_commands(drv),
initialize the work in probe with INIT_WORK(&drv->reinit_work,
rsinput_reinit_work), and in the receive_buf path call
schedule_work(&drv->reinit_work) (then keep the existing goto error flow).
| + drv->boot_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->boot_gpio)) { | ||
| + error = PTR_ERR(drv->boot_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->enable_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->enable_gpio)) { | ||
| + error = PTR_ERR(drv->enable_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); | ||
| + } | ||
| + | ||
| + drv->reset_gpio = | ||
| + devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); | ||
| + if (IS_ERR(drv->reset_gpio)) { | ||
| + error = PTR_ERR(drv->reset_gpio); | ||
| + dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "0002-input-Add-driver-for-RSInput-Gamepad.patch" -type fRepository: armbian/build
Length of output: 139
🏁 Script executed:
cat -n ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | head -360Repository: armbian/build
Length of output: 14562
🏁 Script executed:
cat -n ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | sed -n '353,418p'Repository: armbian/build
Length of output: 2706
🏁 Script executed:
# Search for gpiod_get_optional usage patterns in Linux kernel source
cd /tmp && git clone --depth 1 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2>/dev/null || true
if [ -d linux ]; then
cd linux
# Find examples of proper error handling with gpiod_get_optional
grep -r "devm_gpiod_get_optional" --include="*.c" -A 5 | head -100
fiRepository: armbian/build
Length of output: 5495
🏁 Script executed:
cat > /tmp/test_error_ptr.c << 'EOF'
`#include` <stdio.h>
`#include` <stdint.h>
// Simulate Linux kernel error pointer encoding
`#define` IS_ERR(ptr) (((unsigned long)ptr) >= (unsigned long)-4095)
`#define` PTR_ERR(ptr) ((long)ptr)
int main() {
// Simulate an error pointer
void *error_ptr = (void *)-ENOENT; // -2
// Check if error pointer is non-NULL in boolean context
if (error_ptr) {
printf("Error pointer evaluates to TRUE in boolean context\n");
} else {
printf("Error pointer evaluates to FALSE in boolean context\n");
}
// Check if it's an error
if (IS_ERR(error_ptr)) {
printf("IS_ERR() returns true\n");
printf("Error code: %ld\n", PTR_ERR(error_ptr));
}
return 0;
}
EOF
gcc /tmp/test_error_ptr.c -o /tmp/test_error_ptr && /tmp/test_error_ptrRepository: armbian/build
Length of output: 101
🏁 Script executed:
# Check if there are other dereferences of these GPIO pointers besides gpiod_set_value_cansleep
grep -n "boot_gpio\|enable_gpio\|reset_gpio" ./patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch | tail -20Repository: armbian/build
Length of output: 908
Handle optional GPIO errors by returning instead of continuing.
Error pointers are stored in drv->boot_gpio, drv->enable_gpio, and drv->reset_gpio but only logged with warnings. Later, when these fields are tested with if (drv->*_gpio), error pointers evaluate as true and get dereferenced by gpiod_set_value_cansleep(), causing crashes. Return on error using dev_err_probe() to properly handle -EPROBE_DEFER and other failures, matching the kernel's standard pattern for optional GPIO functions.
🛠️ Safer error handling
- drv->boot_gpio =
- devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH);
- if (IS_ERR(drv->boot_gpio)) {
- error = PTR_ERR(drv->boot_gpio);
- dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error);
- }
+ drv->boot_gpio =
+ devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH);
+ if (IS_ERR(drv->boot_gpio)) {
+ error = PTR_ERR(drv->boot_gpio);
+ return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n");
+ }(Apply the same pattern for enable and reset GPIOs.)
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 333 - 352, The driver stores potential error pointers in
drv->boot_gpio, drv->enable_gpio and drv->reset_gpio via devm_gpiod_get_optional
but only logs warnings, so later checks like if (drv->boot_gpio) can treat error
pointers as valid and gpiod_set_value_cansleep() will crash; change each of
those three acquisition blocks to return on error using
dev_err_probe(&serdev->dev, error, "Unable to get %s gpio\n", "<name>") (or
equivalent dev_err handling) so that if PTR_ERR(drv->*_gpio) indicates failure
you propagate the error (handling -EPROBE_DEFER properly) instead of continuing
and dereferencing the error pointer; apply the same pattern for boot, enable and
reset GPIO acquisitions referenced by devm_gpiod_get_optional and calls to
gpiod_set_value_cansleep().
| + | ||
| + ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + /* Not required, ICNA3512 has DSC always enabled. */ | ||
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | ||
| + if (ret < 0) { | ||
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} |
There was a problem hiding this comment.
Missing error cleanup after PPS and compression mode failures.
If mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode() fails, the function returns without disabling regulators or asserting reset. This leaves hardware in an inconsistent state and leaks power.
Proposed fix: add cleanup on error paths
ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps);
if (ret < 0) {
dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
- return ret;
+ goto err_disable;
}
/* Not required, ICNA3512 has DSC always enabled. */
ret = mipi_dsi_compression_mode(pinfo->dsi, true);
if (ret < 0) {
dev_err(panel->dev, "failed to enable compression mode: %d\n", ret);
- return ret;
+ goto err_disable;
}
return 0;
+
+err_disable:
+ gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
+ regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies);
+ return ret;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + | |
| + ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| + return ret; | |
| + } | |
| + | |
| + /* Not required, ICNA3512 has DSC always enabled. */ | |
| + ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| + if (ret < 0) { | |
| + dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| + return ret; | |
| + } | |
| + | |
| + return 0; | |
| +} | |
| ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); | |
| goto err_disable; | |
| } | |
| /* Not required, ICNA3512 has DSC always enabled. */ | |
| ret = mipi_dsi_compression_mode(pinfo->dsi, true); | |
| if (ret < 0) { | |
| dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); | |
| goto err_disable; | |
| } | |
| return 0; | |
| err_disable: | |
| gpiod_set_value_cansleep(pinfo->reset_gpio, 1); | |
| regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`
around lines 286 - 301, The error paths after calling
mipi_dsi_picture_parameter_set(...) and mipi_dsi_compression_mode(...) currently
return without undoing power/reset state; modify those branches to perform the
same cleanup sequence used on successful shutdown/unprepare (disable regulators
and assert reset) before returning: invoke the existing power-off/unprepare
helper used elsewhere in this driver (e.g., the function that disables
regulators and asserts the panel reset — match the symbol used in this driver
such as icna3512_power_off or chipone_icna3512_unprepare) so both failure
branches cleanly disable supplies and assert reset, then return the error code.
patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
Outdated
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0017-Disable-sdhci_pltfm_free-calling.patch
Outdated
Show resolved
Hide resolved
| +&gpu { | ||
| + zap-shader { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/a740_zap.mbn"; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&pmk8550_gpios { | ||
| + pmk8550_sleep_clk: sleep-clk-state { | ||
| + pins = "gpio3"; | ||
| + function = "func1"; | ||
| + input-disable; | ||
| + output-enable; | ||
| + bias-disable; | ||
| + power-source = <0>; | ||
| + }; | ||
| + | ||
| + pwm_backlight_default: pwm-backlight-default-state { | ||
| + pins = "gpio5"; | ||
| + function = "func3"; | ||
| + input-disable; | ||
| + output-low; | ||
| + output-enable; | ||
| + bias-disable; | ||
| + power-source = <0>; | ||
| + qcom,drive-strength = <2>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&pmk8550_pwm { | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&i2c0 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + pwm_rgb_left: sn3112@54 { | ||
| + compatible = "si-en,sn3112-pwm"; | ||
| + reg = <0x54>; | ||
| + sdb-gpios = <&tlmm 55 GPIO_ACTIVE_LOW>; | ||
| + vdd-supply = <&vdd_mcu_3v3>; | ||
| + #pwm-cells = <1>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&i2c4 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + touchscreen@20 { | ||
| + compatible = "syna,rmi4-i2c"; | ||
| + reg = <0x20>; | ||
| + #address-cells = <1>; | ||
| + #size-cells = <0>; | ||
| + interrupts-extended = <&tlmm 25 0x2008>; | ||
| + | ||
| + pinctrl-names = "default", "sleep"; | ||
| + pinctrl-0 = <&ts_int_default>; | ||
| + pinctrl-1 = <&ts_int_sleep>; | ||
| + | ||
| + vio-supply = <&vreg_l12b_1p8>; | ||
| + | ||
| + syna,startup-delay-ms = <200>; | ||
| + syna,reset-delay-ms = <200>; | ||
| + | ||
| + rmi4-f01@1 { | ||
| + syna,nosleep-mode = <0x1>; | ||
| + reg = <0x1>; | ||
| + }; | ||
| + | ||
| + rmi4-f12@12 { | ||
| + reg = <0x12>; | ||
| + syna,rezero-wait-ms = <20>; | ||
| + syna,clip-x-low = <0>; | ||
| + syna,clip-y-low = <0>; | ||
| + syna,clip-x-high = <1080>; | ||
| + syna,clip-y-high = <1920>; | ||
| + syna,sensor-type = <1>; | ||
| + touchscreen-inverted-x; | ||
| + }; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&i2c12 { | ||
| + clock-frequency = <400000>; | ||
| + status = "okay"; | ||
| + | ||
| + pwm_rgb_right: sn3112@54 { | ||
| + compatible = "si-en,sn3112-pwm"; | ||
| + reg = <0x54>; | ||
| + sdb-gpios = <&tlmm 56 GPIO_ACTIVE_LOW>; | ||
| + vdd-supply = <&vdd_mcu_3v3>; | ||
| + #pwm-cells = <1>; | ||
| + }; | ||
| +}; | ||
| + | ||
| +&remoteproc_adsp { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/adsp.mbn", | ||
| + "qcom/sm8550/ayn/odin2portal/adsp_dtb.mbn"; | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&remoteproc_cdsp { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/cdsp.mbn", | ||
| + "qcom/sm8550/ayn/odin2portal/cdsp_dtb.mbn"; | ||
| + status = "okay"; | ||
| +}; | ||
| + | ||
| +&spk_amp_l { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/aw883xx_acf.bin"; | ||
| +}; | ||
| + | ||
| +&spk_amp_r { | ||
| + firmware-name = "qcom/sm8550/ayn/odin2portal/aw883xx_acf.bin"; | ||
| +}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, list available patches in the directory to confirm file exists
echo "=== Available patches in sm8550-6.18 ==="
ls -1 patch/kernel/archive/sm8550-6.18/ | grep -i odin || echo "No odin patches found"
echo ""
echo "=== Checking the specific patch file ==="
# Check if the patch file exists
test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" && echo "Patch file exists" || echo "Patch file not found"Repository: armbian/build
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Read the specific lines from the patch to verify the firmware paths
if test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"; then
echo "=== Firmware paths in lines 1580-1694 ==="
sed -n '1580,1694p' "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" | grep -E "(firmware-name|odin2)"
fiRepository: armbian/build
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Check all odin-related patches to understand the device variants
echo "=== All odin-related patches ==="
fd -t f "odin" patch/kernel/archive/sm8550-6.18/
echo ""
echo "=== Grep for odin2 vs odin2portal in patches ==="
rg -l "odin2" patch/kernel/archive/sm8550-6.18/ || echo "No matches found"Repository: armbian/build
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Check if there's a sm8550-6.12 version with odin patches to see original naming
echo "=== Checking sm8550-6.12 for odin patches ==="
ls -1 patch/kernel/archive/sm8550-6.12/ 2>/dev/null | grep -i odin || echo "No 6.12 odin patches found"Repository: armbian/build
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Check the 6.12 Odin2 patch to see what firmware paths were originally used
echo "=== Checking 6.12 Odin2 DTS patch for firmware paths ==="
if test -f "patch/kernel/archive/sm8550-6.12/0050_arm64--dts--qcom--Add-AYN-Odin2.patch"; then
echo "File found, checking firmware references:"
grep -n "firmware-name" "patch/kernel/archive/sm8550-6.12/0050_arm64--dts--qcom--Add-AYN-Odin2.patch" | head -10
else
echo "File not found"
fiRepository: armbian/build
Length of output: 484
🏁 Script executed:
#!/bin/bash
# Check the device tree file name and base device node to confirm this is for odin2
echo "=== Checking device tree file name and structure in 6.18 patch ==="
if test -f "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"; then
echo "Device tree file name:"
grep -m1 "^+++ b/arch" "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch"
echo ""
echo "First few lines of device tree content:"
sed -n '1,100p' "patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch" | grep -E "^\+[^+]|device|model|compatible" | head -15
fiRepository: armbian/build
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Check if odin2portal firmware files or device tree exist elsewhere
echo "=== Searching for odin2portal references in all patches ==="
rg -n "odin2portal" patch/kernel/archive/sm8550-6.18/ --glob "*.patch" | head -15
echo ""
echo "=== Check 6.12 patches for odin2portal ==="
rg -n "odin2portal" patch/kernel/archive/sm8550-6.12/ --glob "*.patch" | head -5Repository: armbian/build
Length of output: 3210
Firmware paths incorrectly reference odin2portal in the odin2 device tree.
The qcs8550-ayn-odin2.dts file uses firmware paths pointing to qcom/sm8550/ayn/odin2portal/ instead of qcom/sm8550/ayn/odin2/:
- a740_zap.mbn
- adsp.mbn and adsp_dtb.mbn
- cdsp.mbn and cdsp_dtb.mbn
- aw883xx_acf.bin (speaker amps)
While the patch correctly creates a separate qcs8550-ayn-odin2portal.dts file with the same firmware paths, the base odin2 device should not share the Portal variant's firmware paths. If both devices use identical firmware, this should be explicitly documented or refactored to use a shared path. Otherwise, update all six firmware references in the odin2 section to the correct paths.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`
around lines 1580 - 1694, The firmware paths in the odin2 DTS overlay are
incorrectly pointing to "qcom/sm8550/ayn/odin2portal/"; update the six
firmware-name properties to the correct "qcom/sm8550/ayn/odin2/" path (change
entries in the &gpu zap-shader node, &remoteproc_adsp node, &remoteproc_cdsp
node, and both &spk_amp_l and &spk_amp_r nodes), or alternatively refactor to a
shared firmware path/label and reference that from both odin2 and odin2portal so
the Device Tree doesn't mix Portal-specific paths into the base odin2 device.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 184-199: The code in handle_cmd_commod copies starting at
FRAME_POS_DATA_1 which includes the CMD_COMMOD subcommand byte; change the logic
to skip that byte by starting the version copy at FRAME_POS_DATA_1 + 1, require
payload_length >= 2 for a valid version, compute version_length =
min(payload_length - 1, MCU_VERSION_MAX_LEN - 1), and memcpy from
&data[FRAME_POS_DATA_1 + 1] into mcu_version before null-terminating and logging
with dev_info.
- Around line 370-428: Move the serdev_device_set_client_ops(serdev,
&rsinput_rx_ops) call to before serdev_device_open(serdev) so the serdev ops are
installed prior to opening; ensure drv->serdev is assigned after successful open
as before. Also add serdev_device_close(serdev) on each early return path that
occurs after serdev_device_open(serdev) (the error returns after opening at the
checks that follow: baudrate setup, devm_input_allocate_device/drv->input
allocation, input_register_device, and after device_property reads if they can
fail) so you don't leak the open handle; keep the existing
rsinput_init_commands() error path close behavior. Use the symbols
serdev_device_set_client_ops, serdev_device_open, serdev_device_close,
rsinput_rx_ops, rsinput_init_commands, and drv->serdev to locate and update the
code.
In
`@patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch`:
- Line 348: The MODULE_AUTHOR macro invocation is malformed:
MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); is missing the closing angle
bracket and closing quote; update the MODULE_AUTHOR line (the MODULE_AUTHOR
symbol in the patch) to include the complete author string e.g. add the missing
">" and trailing quote so it becomes MODULE_AUTHOR("Teguh Sobirin
<teguh@sobir.in>");.
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 34-54: sc8280xp_snd_startup enables pdata->i2s_clk before calling
qcom_snd_sdw_startup(), but never disables it if qcom_snd_sdw_startup() fails;
capture the return value from qcom_snd_sdw_startup(substream) into an int ret,
and if ret is negative and the cpu_dai->id case was PRIMARY_MI2S_RX then call
clk_disable_unprepare(pdata->i2s_clk) (or the existing sc8280xp_snd_shutdown
helper if present) before returning ret; ensure you return ret instead of
directly returning qcom_snd_sdw_startup(substream).
In
`@patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch`:
- Around line 99-149: td4328_on() and td4328_off() set/clear MIPI_DSI_MODE_LPM
but don't restore the original dsi->mode_flags on error paths; modify both
functions (td4328_on and td4328_off) to save the original mode_flags at the
start (e.g., orig = dsi->mode_flags) and on any early return due to failure
restore dsi->mode_flags = orig before returning, ensuring the flag state is
always restored on all error paths.
In
`@patch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1713-1715: The interrupt specifiers are inconsistent: the
odin2.dts sdhc_2 node uses 4-cell entries (<GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>)
while odin2portal.dts uses the standard 3-cell form (<GIC_SPI 207
IRQ_TYPE_LEVEL_HIGH>); update the sdhc_2 node to use 3-cell GIC_SPI specifiers
by removing the trailing zero from each interrupts entry so both nodes use
<GIC_SPI N IRQ_TYPE_LEVEL_HIGH>, and verify interrupt-names ("hc_irq","pwr_irq")
still align with the two entries.
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 31-39: The error log in the AW88166_DSP_32_DATA case has a typo
("32r-bit") and should match the 16-bit message format; update the dev_err call
in the AW88166_DSP_32_DATA branch (where aw_dev_dsp_read_32bit is called) to use
"32-bit dsp_data[0x%x] failed" and keep the same arguments ((u32)dsp_addr,
*dsp_data) so the message reads "read dsp_addr[0x%x] 32-bit dsp_data[0x%x]
failed".
- Around line 72-85: The loop doing 16-bit DSP writes must validate buffer
length and use unaligned-safe access: add a pre-check that len is even (return
-EINVAL or jump to error_operation on odd len), replace cpu_to_be16p((u16
*)(data + i)) with get_unaligned_be16(data + i) and write that value, and
include the kernel unaligned header (e.g. `#include` <linux/unaligned.h>). Keep
existing locking and error paths (aw_dev->dsp_lock, regmap_write,
AW88166_DSPMADD_REG, AW88166_DSPMDAT_REG, error_operation) unchanged except for
the new length check and using get_unaligned_be16 instead of cpu_to_be16p.
♻️ Duplicate comments (5)
patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch (1)
19-34: Scope the xHCI readiness skip to affected hardware.Removing the CNR handshake and warning globally means we now write to operational registers even when the host controller reports "not ready," which can cause undefined behavior on other xHCI controllers and removes diagnostics. Please gate this bypass to the specific Odin2/SM8550 controller (e.g., PCI ID match or quirk flag) and keep the default handshake for everyone else.
patch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1580-1694: Firmware paths incorrectly referenceodin2portalin theodin2device tree.The
qcs8550-ayn-odin2.dtsfile uses firmware paths pointing toqcom/sm8550/ayn/odin2portal/instead ofqcom/sm8550/ayn/odin2/for:
- GPU zap-shader (a740_zap.mbn)
- ADSP firmware (adsp.mbn, adsp_dtb.mbn)
- CDSP firmware (cdsp.mbn, cdsp_dtb.mbn)
- Speaker amp firmware (aw883xx_acf.bin)
If both devices share identical firmware, please add a comment documenting this. Otherwise, update the paths to reference the correct
odin2subdirectory.patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (2)
282-284: Incomplete GPIO error handling may cause kernel oops.Only
-EPROBE_DEFERis checked, but other errors fromdevm_gpiod_get()would leave anERR_PTRinpriv->sdb. Subsequentgpiod_set_value_cansleep()calls with this invalid pointer can cause undefined behavior.Proposed fix
priv->sdb = devm_gpiod_get(dev, "sdb", GPIOD_OUT_HIGH); - if (PTR_ERR(priv->sdb) == -EPROBE_DEFER) - return -EPROBE_DEFER; + if (IS_ERR(priv->sdb)) + return PTR_ERR(priv->sdb);
292-311: Resource leak: regulator not disabled on probe failure.If
htr3212_init_regs()orhtr3212_parse_dt()fails,regulator_enable()was already called but is never disabled. This leaks the regulator reference.Proposed fix using goto-based cleanup
ret = regulator_enable(priv->vdd); - if (ret < 0) { + if (ret < 0) return ret; - } gpiod_set_value_cansleep(priv->sdb, 1); usleep_range(10000, 11000); priv->client = client; i2c_set_clientdata(client, priv); ret = htr3212_init_regs(priv); if (ret) - return ret; + goto err_reg_disable; ret = htr3212_parse_dt(dev, priv); if (ret) - return ret; + goto err_reg_disable; return 0; + +err_reg_disable: + gpiod_set_value_cansleep(priv->sdb, 0); + regulator_disable(priv->vdd); + return ret; }patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
20-85: Add<linux/ctype.h>forisxdigit()usage.🛠️ Suggested fix
-#include "btqca.h" +#include "btqca.h" +#include <linux/ctype.h>-/* ath12k/mac.c includes */ +#include <linux/ctype.h>
🧹 Nitpick comments (3)
patch/kernel/archive/sm8550-6.18/0006-dt-bindings-mmc-qcom-Document-level-shifter-flag-for.patch (1)
31-33: Consider adding explicit type for the boolean property.The
qcom,use-level-shifterproperty works as a boolean flag, but for clarity and binding validation, consider addingtype: booleanto the property definition.qcom,use-level-shifter: description: Flag to indicate if platform has level shifter for SD card. + type: booleanpatch/kernel/archive/sm8550-6.18/0016-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1574-1574: Inconsistent indentation.Line 1574 uses spaces for indentation while the rest of the file uses tabs. This should be tabs for consistency with kernel DTS style.
- backlight = <&backlight>; + backlight = <&backlight>;patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (1)
136-141: Simplify return statement.The
if (ret) return ret; return 0;pattern can be simplified.static int htr3212_reset_regs(struct htr3212_priv *priv) { - int ret; - - ret = htr3212_write(priv, HTR3212_RESET, 0x00); - if (ret) - return ret; - - return 0; + return htr3212_write(priv, HTR3212_RESET, 0x00); }
| +static void handle_cmd_commod(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { | ||
| + switch (data[FRAME_POS_DATA_1]) { | ||
| + case DATA_COMMOD_VERSION: | ||
| + if (payload_length >= 1) { | ||
| + char mcu_version[MCU_VERSION_MAX_LEN] = {0}; | ||
| + size_t version_length = payload_length; | ||
| + if (version_length > MCU_VERSION_MAX_LEN - 1) { | ||
| + version_length = MCU_VERSION_MAX_LEN - 1; | ||
| + } | ||
| + memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); | ||
| + mcu_version[version_length] = '\0'; | ||
| + dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version); | ||
| + } else { | ||
| + dev_err(&drv->serdev->dev, "Invalid MCU version response length\n"); | ||
| + } | ||
| + break; |
There was a problem hiding this comment.
Skip the CMD_COMMOD subcommand byte when building the MCU version string.
mcu_version currently includes the subcommand byte at FRAME_POS_DATA_1, which is not part of the version string.
🛠️ Suggested fix
case DATA_COMMOD_VERSION:
- if (payload_length >= 1) {
+ if (payload_length >= 2) {
char mcu_version[MCU_VERSION_MAX_LEN] = {0};
- size_t version_length = payload_length;
+ size_t version_length = payload_length - 1;
if (version_length > MCU_VERSION_MAX_LEN - 1) {
version_length = MCU_VERSION_MAX_LEN - 1;
}
- memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length);
+ memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
mcu_version[version_length] = '\0';
dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version);🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 184 - 199, The code in handle_cmd_commod copies starting at
FRAME_POS_DATA_1 which includes the CMD_COMMOD subcommand byte; change the logic
to skip that byte by starting the version copy at FRAME_POS_DATA_1 + 1, require
payload_length >= 2 for a valid version, compute version_length =
min(payload_length - 1, MCU_VERSION_MAX_LEN - 1), and memcpy from
&data[FRAME_POS_DATA_1 + 1] into mcu_version before null-terminating and logging
with dev_info.
patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch
Show resolved
Hide resolved
| +static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | ||
| +{ | ||
| + unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | ||
| + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | ||
| + struct snd_soc_pcm_runtime *rtd = substream->private_data; | ||
| + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | ||
| + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | ||
| + struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | ||
| + | ||
| + switch (cpu_dai->id) { | ||
| + case PRIMARY_MI2S_RX: | ||
| + clk_prepare_enable(pdata->i2s_clk); | ||
| + snd_soc_dai_set_fmt(cpu_dai, fmt); | ||
| + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | ||
| + break; | ||
| + default: | ||
| + break; | ||
| + } | ||
| + | ||
| + return qcom_snd_sdw_startup(substream); | ||
| +} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat "patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch"Repository: armbian/build
Length of output: 3220
🏁 Script executed:
rg -A 10 "qcom_snd_sdw_startup" --type c | head -40Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for similar startup error handling in ASoC drivers
rg -B 5 -A 10 "\.startup.*=" sound/soc/qcom/ --type c | head -60Repository: armbian/build
Length of output: 112
🏁 Script executed:
# Search all patches for qcom_snd_sdw_startup references
rg "qcom_snd_sdw_startup" patch/Repository: armbian/build
Length of output: 578
🏁 Script executed:
# Look at other ASoC startup handlers to understand error handling patterns
rg -B 3 -A 15 "static.*startup.*snd_pcm_substream" patch/ --type patch | head -80Repository: armbian/build
Length of output: 86
🏁 Script executed:
# Search for qcom_snd_sdw_startup definition or declaration
grep -r "qcom_snd_sdw_startup" patch/ | head -20Repository: armbian/build
Length of output: 578
🏁 Script executed:
# Look for the function that defines qcom_snd_sdw_startup
grep -r "int.*qcom_snd_sdw_startup\|qcom_snd_sdw_startup.*int" patch/Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check the 6.12 version of the same patch for comparison
cat patch/kernel/archive/sm8550-6.12/0036_ASoC--qcom--sc8280xp-Add-support-for-Primary-I2S.patch | grep -A 50 "sc8280xp_snd_startup"Repository: armbian/build
Length of output: 2129
🏁 Script executed:
# Search for qcom_snd_sdw_startup in kernel documentation or other files
find patch/ -name "*.c" -o -name "*.h" | xargs grep -l "qcom_snd_sdw_startup" 2>/dev/null | head -5Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Search in patches for qcom_snd_sdw or common.c patterns
grep -r "qcom_snd_sdw" patch/ | grep -i "static.*int\|return.*error\|fail" | head -10Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if there are other qcom drivers with similar error handling
grep -r "clk_prepare_enable" patch/kernel/archive/sm8550-6.18/ | head -5Repository: armbian/build
Length of output: 325
🏁 Script executed:
# Look for examples of error handling in ASoC startup handlers in other patches
grep -B 5 -A 15 "static.*int.*startup" patch/kernel/archive/sm8550-6.18/*.patch | grep -A 15 -B 5 "return.*ret\|if.*ret" | head -40Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Check if qcom_snd_sdw_startup is defined elsewhere in patches
grep -r "qcom_snd_sdw_startup\|def.*qcom_snd" patch/ | grep -v "\.patch:" | head -20Repository: armbian/build
Length of output: 39
🏁 Script executed:
# Look for error paths after resource allocation in ASoC drivers
grep -B 3 -A 8 "clk_prepare_enable" patch/kernel/archive/sm8550-6.18/*.patch | head -50Repository: armbian/build
Length of output: 2788
🏁 Script executed:
# Search for standard ASoC pattern - check if shutdown is paired with startup in framework
grep -r "startup\|shutdown" patch/kernel/archive/sm8550-6.18/*.patch | grep -i "snd_soc_ops\|\.startup\|\.shutdown" | head -20Repository: armbian/build
Length of output: 666
🏁 Script executed:
# Look for error handling patterns after clock enable in kernel drivers
grep -B 2 -A 5 "clk_prepare_enable" patch/ -r | grep -B 5 -A 5 "if\|return\|error" | head -50Repository: armbian/build
Length of output: 4812
🏁 Script executed:
# Check if clk_prepare_enable can return an error value
grep -r "clk_prepare_enable" patch/ | grep "ret\|error\|if\|check" | head -15Repository: armbian/build
Length of output: 1767
🏁 Script executed:
# Look for the kernel source to understand clk_prepare_enable signature
web_search "clk_prepare_enable return value kernel documentation"Repository: armbian/build
Length of output: 107
🌐 Web query:
clk_prepare_enable return value kernel clock framework
💡 Result:
clk_prepare_enable(struct clk *clk) returns an int status:
0on success- Negative
-errnoon failure (it returns the error fromclk_prepare()orclk_enable()). Ifclk_enable()fails, it callsclk_unprepare(clk)to roll back and then returns the enable error. [1]
It is intended for non-atomic (sleepable) context, because clk_prepare() may sleep. [1][2]
Sources: [1] [2]
🌐 Web query:
ASoC snd_soc_ops startup shutdown error handling framework
💡 Result:
In ASoC, the framework-level error handling for struct snd_soc_ops/DAI ops startup()/shutdown() is implemented in sound/soc/soc-pcm.c:
-
Call order on open (
soc_pcm_open):snd_soc_dai_startup(cpu_dai, substream)- open all rtd components (
soc_pcm_components_open) snd_soc_dai_startup(codec_dai, substream)for each codec DAI- optional machine/link
rtd->dai_link->ops->startup(substream)
If any step fails, the function jumps to rollback labels that unwind only what was successfully started/opened. [1][2]
-
Rollback / unwind guarantees on error:
- If machine/link
startup()ran and later checks fail, machine/linkshutdown()is called (config_err:path). [2] - If any codec DAI
startup()fails (or later steps fail), already-started codec DAIs are shut down in reverse via a rollback iterator, then components are closed, then CPU DAI is shut down. [2] - Component open/close is written to support partial success: a later patch explicitly fixed/centralized “close already-opened components on error” behavior so partial opens are cleaned up correctly. [4]
- If machine/link
-
Normal close (
soc_pcm_close):- Deactivates runtime, mutes, then calls
snd_soc_dai_shutdown(cpu_dai, ...),snd_soc_dai_shutdown(codec_dai, ...), optional machine/linkshutdown(), thensoc_pcm_components_close(...). [3]
- Deactivates runtime, mutes, then calls
Practical guidance for driver authors:
startup()must return< 0on failure; the core will unwind and callshutdown()only for parts that were successfully started. [1][2]shutdown()must be safe as a cleanup hook (typically tolerate partial setup, and be effectively idempotent with respect to the resources you acquired instartup()), because it’s used both on normal close and on open-time error rollback. [2][3]
Sources: Linux ASoC core (soc_pcm_open error labels and unwind paths; soc_pcm_close shutdown ordering), plus the patch discussion about fixing component open/close error unwind. [1][2][3][4]
Add error-path cleanup for i2s_clk if qcom_snd_sdw_startup() fails.
The clock is enabled before calling qcom_snd_sdw_startup(), which can fail. If it does, shutdown() is not called, leaving the clock enabled. Capture the return value and disable the clock on error before returning.
Proposed fix
static int sc8280xp_snd_startup(struct snd_pcm_substream *substream)
{
unsigned int fmt = SND_SOC_DAIFMT_BP_FP;
unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card);
struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
+ int ret;
switch (cpu_dai->id) {
case PRIMARY_MI2S_RX:
clk_prepare_enable(pdata->i2s_clk);
snd_soc_dai_set_fmt(cpu_dai, fmt);
snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
break;
default:
break;
}
- return qcom_snd_sdw_startup(substream);
+ ret = qcom_snd_sdw_startup(substream);
+ if (ret && cpu_dai->id == PRIMARY_MI2S_RX)
+ clk_disable_unprepare(pdata->i2s_clk);
+ return ret;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | |
| +{ | |
| + unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | |
| + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | |
| + struct snd_soc_pcm_runtime *rtd = substream->private_data; | |
| + struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | |
| + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | |
| + struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | |
| + | |
| + switch (cpu_dai->id) { | |
| + case PRIMARY_MI2S_RX: | |
| + clk_prepare_enable(pdata->i2s_clk); | |
| + snd_soc_dai_set_fmt(cpu_dai, fmt); | |
| + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| + break; | |
| + default: | |
| + break; | |
| + } | |
| + | |
| + return qcom_snd_sdw_startup(substream); | |
| +} | |
| static int sc8280xp_snd_startup(struct snd_pcm_substream *substream) | |
| { | |
| unsigned int fmt = SND_SOC_DAIFMT_BP_FP; | |
| unsigned int codec_dai_fmt = SND_SOC_DAIFMT_BC_FC | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S; | |
| struct snd_soc_pcm_runtime *rtd = substream->private_data; | |
| struct sc8280xp_snd_data *pdata = snd_soc_card_get_drvdata(rtd->card); | |
| struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); | |
| struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0); | |
| int ret; | |
| switch (cpu_dai->id) { | |
| case PRIMARY_MI2S_RX: | |
| clk_prepare_enable(pdata->i2s_clk); | |
| snd_soc_dai_set_fmt(cpu_dai, fmt); | |
| snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt); | |
| break; | |
| default: | |
| break; | |
| } | |
| ret = qcom_snd_sdw_startup(substream); | |
| if (ret && cpu_dai->id == PRIMARY_MI2S_RX) | |
| clk_disable_unprepare(pdata->i2s_clk); | |
| return ret; | |
| } |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`
around lines 34 - 54, sc8280xp_snd_startup enables pdata->i2s_clk before calling
qcom_snd_sdw_startup(), but never disables it if qcom_snd_sdw_startup() fails;
capture the return value from qcom_snd_sdw_startup(substream) into an int ret,
and if ret is negative and the cpu_dai->id case was PRIMARY_MI2S_RX then call
clk_disable_unprepare(pdata->i2s_clk) (or the existing sc8280xp_snd_shutdown
helper if present) before returning ret; ensure you return ret instead of
directly returning qcom_snd_sdw_startup(substream).
| +static int td4328_on(struct td4328 *ctx) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = ctx->dsi; | ||
| + struct device *dev = &dsi->dev; | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags |= MIPI_DSI_MODE_LPM; | ||
| + | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x00); | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xb3, 0x31); | ||
| + mipi_dsi_dcs_write_seq(dsi, 0xd6, 0x00); | ||
| + | ||
| + ret = mipi_dsi_dcs_exit_sleep_mode(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to exit sleep mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(70); | ||
| + | ||
| + ret = mipi_dsi_dcs_set_display_on(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to set display on: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int td4328_off(struct td4328 *ctx) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = ctx->dsi; | ||
| + struct device *dev = &dsi->dev; | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; | ||
| + | ||
| + ret = mipi_dsi_dcs_set_display_off(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to set display off: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(50); | ||
| + | ||
| + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); | ||
| + if (ret < 0) { | ||
| + dev_err(dev, "Failed to enter sleep mode: %d\n", ret); | ||
| + return ret; | ||
| + } | ||
| + msleep(120); | ||
| + | ||
| + return 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch" | sed -n '90,160p'Repository: armbian/build
Length of output: 170
🏁 Script executed:
# Search for similar panel drivers to check their MIPI_DSI_MODE_LPM handling
fd -type f -name "*.c" -path "*/panel/*" | head -10Repository: armbian/build
Length of output: 227
🏁 Script executed:
# Look for MIPI_DSI_MODE_LPM usage patterns in the kernel
rg "MIPI_DSI_MODE_LPM" -A 3 -B 3 | head -100Repository: armbian/build
Length of output: 10293
🏁 Script executed:
sed -n '90,160p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 1709
🏁 Script executed:
# Also check if there's any cleanup on error or other LPM handling
grep -n "mode_flags\|LPM" "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 219
🏁 Script executed:
# Find the context around line 252 to see the initial mode_flags setting
sed -n '245,260p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 507
🏁 Script executed:
# Check the prepare/unprepare functions to see the calling context
sed -n '160,250p' "patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch"Repository: armbian/build
Length of output: 2409
Restore LPM flag on error paths in td4328_on() and td4328_off().
The functions rely on balanced pairs (prepare calls on(), unprepare calls off()), but td4328_on() sets MIPI_DSI_MODE_LPM and fails without restoring it. If initialization fails after that flag change, subsequent DSI operations may use incorrect mode flags. Save and restore mode_flags within each function to ensure cleanup on all paths:
Suggested pattern
static int td4328_on(struct td4328 *ctx)
{
struct mipi_dsi_device *dsi = ctx->dsi;
struct device *dev = &dsi->dev;
+ unsigned long mode_flags = dsi->mode_flags;
int ret;
dsi->mode_flags |= MIPI_DSI_MODE_LPM;
@@
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
if (ret < 0) {
dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
- return ret;
+ goto restore_flags;
}
@@
ret = mipi_dsi_dcs_set_display_on(dsi);
if (ret < 0) {
dev_err(dev, "Failed to set display on: %d\n", ret);
- return ret;
+ goto restore_flags;
}
+ ret = 0;
+restore_flags:
+ dsi->mode_flags = mode_flags;
return ret;
}Apply the same pattern in td4328_off().
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch`
around lines 99 - 149, td4328_on() and td4328_off() set/clear MIPI_DSI_MODE_LPM
but don't restore the original dsi->mode_flags on error paths; modify both
functions (td4328_on and td4328_off) to save the original mode_flags at the
start (e.g., orig = dsi->mode_flags) and on any early return due to failure
restore dsi->mode_flags = orig before returning, ensuring the flag state is
always restored on all error paths.
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
Show resolved
Hide resolved
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | ||
| + (u32)dsp_addr, *dsp_data); | ||
| break; | ||
| case AW88166_DSP_32_DATA: | ||
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | ||
| if (ret) | ||
| - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit failed", (u32)dsp_addr); | ||
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | ||
| + (u32)dsp_addr, *dsp_data); |
There was a problem hiding this comment.
Fix typo in error log ("32r-bit").
🛠️ Proposed fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed",
+ dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed",
(u32)dsp_addr, *dsp_data);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | |
| + (u32)dsp_addr, *dsp_data); | |
| break; | |
| case AW88166_DSP_32_DATA: | |
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | |
| if (ret) | |
| - dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit failed", (u32)dsp_addr); | |
| + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", | |
| + (u32)dsp_addr, *dsp_data); | |
| dev_err(aw_dev->dev, "read dsp_addr[0x%x] 16-bit dsp_data[0x%x] failed", | |
| (u32)dsp_addr, *dsp_data); | |
| break; | |
| case AW88166_DSP_32_DATA: | |
| ret = aw_dev_dsp_read_32bit(aw_dev, dsp_addr, dsp_data); | |
| if (ret) | |
| dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", | |
| (u32)dsp_addr, *dsp_data); |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 31 - 39, The error log in the AW88166_DSP_32_DATA case has a typo
("32r-bit") and should match the 16-bit message format; update the dev_err call
in the AW88166_DSP_32_DATA branch (where aw_dev_dsp_read_32bit is called) to use
"32-bit dsp_data[0x%x] failed" and keep the same arguments ((u32)dsp_addr,
*dsp_data) so the message reads "read dsp_addr[0x%x] 32-bit dsp_data[0x%x]
failed".
| + __be16 reg_val; | ||
| + | ||
| + mutex_lock(&aw_dev->dsp_lock); | ||
| + /* i2c write */ | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + for (i = 0; i < len; i += 2) { | ||
| + reg_val = cpu_to_be16p((u16 *)(data + i)); | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | ||
| + (u16)reg_val); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the patch file
find . -name "0017-HACK-Revert-back-to-6.12-aw88166-driver.patch" -type fRepository: armbian/build
Length of output: 142
🏁 Script executed:
# Read the patch file to examine the code at lines 72-85 and surrounding context
wc -l patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patchRepository: armbian/build
Length of output: 144
🏁 Script executed:
# Read the patch file to understand the complete function and context
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '50,110p'Repository: armbian/build
Length of output: 2115
🏁 Script executed:
# Search for the function that contains this code - look for the function definition
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '1,80p'Repository: armbian/build
Length of output: 3342
🏁 Script executed:
# Look for the function definition - search backwards from line 72
grep -n "^-\|^+\|^@@" patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | head -20Repository: armbian/build
Length of output: 964
🏁 Script executed:
# Search for callers of aw_dev_dsp_update_container
rg "aw_dev_dsp_update_container" --type cRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Search for information about cpu_to_be16p in the codebase or check kernel behavior
# Also look for get_unaligned_be16 usage as comparison
rg "cpu_to_be16p|get_unaligned_be16" --type cRepository: armbian/build
Length of output: 39
🏁 Script executed:
# Let me search within the patch file for function definitions and callers
grep -n "aw_dev_dsp_update_container" patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patchRepository: armbian/build
Length of output: 125
🏁 Script executed:
# Look at more of the patch to understand the full function and how it's called
cat -n patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch | sed -n '40,95p'Repository: armbian/build
Length of output: 2051
🌐 Web query:
cpu_to_be16p kernel unaligned access safety
💡 Result:
cpu_to_be16p(p) (and the other *_to_*p() “pointer” byteorder helpers) are intended to be usable on unaligned pointers, but may incur a performance penalty on some architectures; for unaligned data, the kernel explicitly recommends using the unaligned-access helpers instead. [1]
So, for portable + clearly-unaligned-safe code, prefer:
get_unaligned_be16(p)(orbe16_to_cpu(get_unaligned((__be16 *)p))in older-style code), andput_unaligned_be16(val, p)for stores,
because the kernel documentation states the standard way to avoid unaligned access problems is get_unaligned() / put_unaligned() (and friends). [2]
Rule of thumb: use cpu_to_be16p() only when you know the pointer is suitably aligned (or you accept possible arch-dependent slow paths); use get_unaligned_be16() when alignment is uncertain. [1][2]
Sources: [1] Linux include/linux/byteorder/generic.h comment on ...p macros and unaligned data, [2] Linux “Unaligned Memory Accesses” documentation.
🌐 Web query:
get_unaligned_be16 linux kernel unaligned memory access
💡 Result:
get_unaligned_be16() is the Linux-kernel helper for safely reading a 16-bit big-endian value from memory that may be unaligned (i.e., the pointer isn’t guaranteed to be 2-byte aligned). It exists to avoid alignment faults (or slow fixups) on architectures that can’t efficiently do unaligned loads. [1]
Typical use
`#include` <linux/unaligned.h>
u16 v = get_unaligned_be16(p); /* p points to 2 bytes in big-endian order */What it does
On architectures where unaligned loads aren’t assumed efficient, the kernel often implements it by byte assembling, equivalent to:
(u16)(p[0] << 8 | p[1])so it never performs a potentially-faulting unaligned *(u16 *)p load. [2]
On architectures that do advertise efficient unaligned access (CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS), the generic unaligned framework may select a more direct implementation, but the API contract stays the same: works even if p is unaligned. [3]
Why you’d use it
Kernel docs explicitly recommend get_unaligned()/put_unaligned() (and the endian-specific variants like get_unaligned_be16) as the portable way to avoid unaligned accesses when parsing “off-the-wire” / packed data. [1]
Sources: Linux kernel unaligned-access documentation [1], example byte-shift implementation of get_unaligned_be16 [2], and how the kernel selects implementations based on endianness / unaligned-access capability [3].
Add length validation and use safe unaligned access for DSP writes.
The 16-bit write loop reads data + i as u16 * without alignment guarantees or odd-length checks. Per kernel documentation, cpu_to_be16p() is not recommended for unaligned data; use get_unaligned_be16() instead. Additionally, if len is odd, the final iteration will read beyond the buffer.
Suggested fix
+ if (len & 0x1)
+ return -EINVAL;
+
for (i = 0; i < len; i += 2) {
- reg_val = cpu_to_be16p((u16 *)(data + i));
+ reg_val = get_unaligned_be16(data + i);
ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG,
(u16)reg_val);Add the required include:
+#include <linux/unaligned.h>🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0017-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 72 - 85, The loop doing 16-bit DSP writes must validate buffer
length and use unaligned-safe access: add a pre-check that len is even (return
-EINVAL or jump to error_operation on odd len), replace cpu_to_be16p((u16
*)(data + i)) with get_unaligned_be16(data + i) and write that value, and
include the kernel unaligned header (e.g. `#include` <linux/unaligned.h>). Keep
existing locking and error paths (aw_dev->dsp_lock, regmap_write,
AW88166_DSPMADD_REG, AW88166_DSPMDAT_REG, error_operation) unchanged except for
the new length check and using get_unaligned_be16 instead of cpu_to_be16p.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch`:
- Around line 45-66: The calls to clk_prepare_enable(pdata->i2s_clk) and
clk_disable_unprepare(pdata->i2s_clk) must be guarded against NULL because
devm_clk_get_optional() may return NULL; update the startup path (where
clk_prepare_enable is called in the PRIMARY_MI2S_RX case) and the shutdown path
(sc8280xp_snd_shutdown where clk_disable_unprepare is called) to check
pdata->i2s_clk for non-NULL before calling clk_prepare_enable or
clk_disable_unprepare so we avoid dereferencing a NULL clock pointer.
♻️ Duplicate comments (17)
patch/kernel/archive/sm8550-6.18/0014-HACK-fix-usb-boot-hang.patch (1)
19-30: Scope the xHCI readiness skip to the affected controller only.
Removing the CNR handshake and warning globally can affect other xHCI controllers; gate the bypass by quirk/PCI ID and keep the default path for everyone else.patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch (1)
44-56: Ensure 37.5 MHz exists in the OPP table whenqcom,use-level-shifteris set.
If the OPP table lacks 37.5 MHz,dev_pm_opp_set_rate()will fail and HS mode will be rejected on affected devices.patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch (1)
34-53: Startup path still needs error handling and cleanup.
The return values ofclk_prepare_enable()/snd_soc_dai_set_fmt()aren’t checked, andi2s_clkisn’t disabled ifqcom_snd_sdw_startup()fails.patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch (1)
99-149: Restore DSI mode flags on all exit paths.Line 105 sets
MIPI_DSI_MODE_LPMand Lines 112/118/136/143 can return early without restoring the originalmode_flags, leaving the DSI device in the wrong mode after failures.🐛 Suggested fix (apply same pattern to
td4328_off())static int td4328_on(struct td4328 *ctx) { struct mipi_dsi_device *dsi = ctx->dsi; struct device *dev = &dsi->dev; + unsigned long mode_flags = dsi->mode_flags; int ret; dsi->mode_flags |= MIPI_DSI_MODE_LPM; @@ ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret < 0) { dev_err(dev, "Failed to exit sleep mode: %d\n", ret); - return ret; + goto restore_flags; } @@ ret = mipi_dsi_dcs_set_display_on(dsi); if (ret < 0) { dev_err(dev, "Failed to set display on: %d\n", ret); - return ret; + goto restore_flags; } - return 0; + ret = 0; +restore_flags: + dsi->mode_flags = mode_flags; + return ret; }patch/kernel/archive/sm8550-6.18/0017-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (2)
1591-1705: Odin2 DTS points at Portal firmware paths.Line 1593 and Lines 1688–1704 reference
.../odin2portal/...paths inside the Odin2 DTS. Unless Odin2 and Odin2 Portal intentionally share identical firmware directories, these should be.../odin2/...(or a shared label) to avoid loading the wrong blobs.🐛 Example fix (apply to ADSP/CDSP/amp paths too)
&gpu { zap-shader { - firmware-name = "qcom/sm8550/ayn/odin2portal/a740_zap.mbn"; + firmware-name = "qcom/sm8550/ayn/odin2/a740_zap.mbn"; }; };
1720-1725: Interrupt specifier cell count mismatch.Line 1724–1725 uses 4-cell
GIC_SPIspecifiers, while the Portal DTS at Line 2068–2069 uses the standard 3-cell form. These should be consistent with the GIC binding.🐛 Suggested fix
- interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH 0>, - <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH 0>; + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (6)
184-195: Skip the subcommand byte when building the MCU version string.Line 185 copies from
FRAME_POS_DATA_1, which includes the CMD_COMMOD subcommand. The version string should start atFRAME_POS_DATA_2.🐛 Suggested fix
- if (payload_length >= 1) { + if (payload_length >= 2) { char mcu_version[MCU_VERSION_MAX_LEN] = {0}; - size_t version_length = payload_length; + size_t version_length = payload_length - 1; if (version_length > MCU_VERSION_MAX_LEN - 1) { version_length = MCU_VERSION_MAX_LEN - 1; } - memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); + memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
209-237: Makeprev_statesper-device instead of static.Line 211 declares
static unsigned long prev_states, which leaks state across device instances. Move it intostruct rsinput_driverand referencedrv->prev_stateshere.
281-311: RX path assumes full frame per callback.Lines 295–311 copy a single callback into
rx_bufand validate immediately, which drops fragmented UART frames. Buffer and assemble frames across callbacks before checksum validation.
306-308: Avoid sleeping in the serdev receive callback.Line 307 calls
rsinput_init_commands()(which sleeps) insidereceive_buf. Defer to a workqueue or threaded context.🐛 Suggested fix
- if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); - goto error; - } + if (computed_checksum != received_checksum) { + schedule_work(&drv->reinit_work); + goto error; + }Additional changes needed (outside this hunk):
- Add
struct work_struct reinit_work;tostruct rsinput_driver.- Initialize with
INIT_WORK(&drv->reinit_work, rsinput_reinit_work);in probe.- Implement
rsinput_reinit_work()to callrsinput_init_commands().
333-352: Return on optional GPIO errors to avoid deref of error pointers.Lines 333–352 log
IS_ERR()but continue; laterif (drv->*_gpio)treats error pointers as valid and dereferences them. Return on error usingdev_err_probe().🐛 Suggested fix (apply to boot/enable/reset)
drv->boot_gpio = devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); if (IS_ERR(drv->boot_gpio)) { error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n"); }
370-423: Set serdev ops before open and close on all error paths.Line 370 opens the device before
serdev_device_set_client_ops(), and several early returns after open (Lines 381/387/420) leak the open handle.🐛 Suggested fix
- error = serdev_device_open(serdev); + serdev_device_set_client_ops(serdev, &rsinput_rx_ops); + error = serdev_device_open(serdev); if (error) return dev_err_probe(&serdev->dev, error, "Unable to open UART device\n"); @@ error = serdev_device_set_baudrate(serdev, 115200); if (error < 0) - return dev_err_probe(&serdev->dev, error, "Failed to set up host baud rate\n"); + goto err_close; @@ drv->input = devm_input_allocate_device(&serdev->dev); - if (!drv->input) - return -ENOMEM; + if (!drv->input) { + error = -ENOMEM; + goto err_close; + } @@ error = input_register_device(drv->input); if (error) - return error; - - serdev_device_set_client_ops(serdev, &rsinput_rx_ops); + goto err_close; @@ error = rsinput_init_commands(drv); - if (error < 0) { - serdev_device_close(serdev); - return error; - } + if (error < 0) + goto err_close; return 0; + +err_close: + serdev_device_close(serdev); + return error;patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (2)
35-39: Fix typo in error log ("32r-bit").Line 38 has a typo in the 32-bit DSP read error message.
🛠️ Suggested fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);
72-85: Guard odd-length writes and use unaligned-safe access.Lines 79–82 cast
data + itou16 *without alignment guarantees and don’t check oddlen. This can read past the buffer and fault on strict-alignment arches.🐛 Suggested fix
+ if (len & 0x1) + return -EINVAL; + for (i = 0; i < len; i += 2) { - reg_val = cpu_to_be16p((u16 *)(data + i)); + reg_val = get_unaligned_be16(data + i); ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, (u16)reg_val);Additional include needed (outside this hunk):
+#include <linux/unaligned.h>patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (3)
18-21: Add<linux/ctype.h>forisxdigit().Line 71 (btqca.c) and Line 209 (ath12k/mac.c) use
isxdigit()but no ctype include is present, which breaks the build on stricter compilers.🐛 Suggested fix
`#include` "btqca.h" +#include <linux/ctype.h>`#include` "mac.h" +#include <linux/ctype.h>Also applies to: 153-154
116-135: Keep the NVM fallback when serial-derived BD_ADDR fails.Lines 118–129 hard-fail if serial generation or setting fails, removing the previous fallback path and risking BT bring-up failures when socinfo is unavailable.
🐛 Suggested fallback
/* Generate BD_ADDR from qcom_serial_number */ err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); if (err) { - bt_dev_err(hdev, "Failed to generate BD_ADDR from serial number"); - return err; + bt_dev_warn(hdev, "Serial BD_ADDR unavailable, falling back to NVM"); + err = qca_check_bdaddr(hdev, &config); + if (err) + return err; + } else { + /* Set the generated BD_ADDR */ + err = qca_set_bdaddr(hdev, &generated_bdaddr); + if (err) { + bt_dev_err(hdev, "Failed to set serial BD_ADDR"); + err = qca_check_bdaddr(hdev, &config); + if (err) + return err; + } else { + bacpy(&hdev->public_addr, &generated_bdaddr); + bacpy(&hdev->bdaddr, &generated_bdaddr); + bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); + } }
252-268: Preserve per‑pdev MACs and avoid duplicates.Lines 252–268 override valid per‑pdev MACs and don’t offset by
pdev_idxon the serial‑derived path, so multi‑radio devices can get duplicate MACs.🐛 Suggested fix
- ret = generate_macaddr_from_serial(ar, &generated_macaddr); - if (ret) { - if (ar->ab->pdevs_macaddr_valid) { - ether_addr_copy(ar->mac_addr, pdev->mac_addr); - } else { - ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); - ar->mac_addr[4] += ar->pdev_idx; - } - } else { - ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); - ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); - } + if (ar->ab->pdevs_macaddr_valid) { + ether_addr_copy(ar->mac_addr, pdev->mac_addr); + } else { + ret = generate_macaddr_from_serial(ar, &generated_macaddr); + if (!ret) { + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); + ar->mac_addr[4] += ar->pdev_idx; + ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr); + } else { + ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); + ar->mac_addr[4] += ar->pdev_idx; + } + }
patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch
Outdated
Show resolved
Hide resolved
kasimling
force-pushed
the
odin2_k618
branch
2 times, most recently
from
e4348b8 to
04d437f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch`:
- Around line 296-330: The probe enables the regulator with
regulator_enable(priv->vdd) but on subsequent failures (devm_regmap_init_i2c,
pwmchip_add, sn3112_write_all) it returns without disabling it; add explicit
cleanup calls to disable the regulator (regulator_disable(priv->vdd)) on every
error path after regulator_enable succeeds. Specifically: after
devm_regmap_init_i2c fails, call regulator_disable(priv->vdd) before returning;
if pwmchip_add fails call regulator_disable(priv->vdd) before returning; if
sn3112_write_all fails (after pwmchip_add succeeded) call pwmchip_remove(chip)
then regulator_disable(priv->vdd) before returning (also clear
i2c_set_clientdata/client if needed). Ensure these cleanup calls are placed
around the failing symbols devm_regmap_init_i2c, pwmchip_add, and
sn3112_write_all.
- Around line 239-243: pwm_apply currently ignores return values from
sn3112_set_en_reg, sn3112_set_val_reg, and sn3112_write_reg which can fail;
change pwm_apply to capture and propagate errors: declare an int ret, call each
of sn3112_set_en_reg(priv,...), sn3112_set_val_reg(...), and
sn3112_write_reg(...), checking ret after each call and if ret < 0 jump to an
unlock-and-return path that calls mutex_unlock(&priv->lock) before returning the
error; ensure successful path returns 0; reference the existing
mutex_lock(&priv->lock), sn3112_set_en_reg, sn3112_set_val_reg,
sn3112_write_reg, and SN3112_REG_APPLY to locate the code.
- Around line 289-294: The devm_gpiod_get_optional call storing priv->sdb
currently only checks PTR_ERR(priv->sdb) == -EPROBE_DEFER and leaves other
ERR_PTR values unhandled; change the check to use IS_ERR(priv->sdb) and return
PTR_ERR(priv->sdb) if true (handle -EPROBE_DEFER and other errors uniformly) so
priv->sdb never remains an error pointer for later if (priv->sdb) checks; update
the block around devm_gpiod_get_optional/priv->sdb accordingly to mirror the
error handling used by
devm_pwmchip_alloc/devm_regulator_get/devm_regmap_init_i2c.
- Around line 129-136: The code currently casts byte-aligned pwm_en_reg elements
to ulong* and calls set_bit()/clear_bit(), which can read/write past the 3-byte
buffer; replace those calls with byte-level operations on priv->pwm_en_reg[reg]
(e.g., set or clear the (1U << bit) bit in the uint8_t element) and ensure you
validate or assert that reg is within the pwm_en_reg array and bit is < 8 before
modifying; keep the surrounding dev_dbg calls but remove the
set_bit()/clear_bit() casts so we no longer rely on unsigned long word
operations on a u8 buffer.
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1731-1732: The device tree uses the property name
qcom,uses_level_shifter (snake_case + plural) but the driver expects
qcom,use-level-shifter (kebab-case singular), so update the DTS nodes that added
qcom,uses_level_shifter to use qcom,use-level-shifter instead (apply the same
rename in the second node occurrence around the 2075–2076 area); leave
qcom,dll_lock_bist_fail_wa as-is unless the driver expects a different name.
Ensure both device nodes use the exact qcom,use-level-shifter string so the
driver reads the flag correctly.
- Around line 1597-1616: The patch defines a duplicate label pmk8550_sleep_clk
which causes dtc duplicate‑label errors; remove the local pmk8550_sleep_clk node
or convert it to an override using the existing label via &pmk8550_sleep_clk {
... } inside the &pmk8550_gpios block; ensure you keep or add only the
pwm_backlight_default node (or rename it if needed) and do not re-declare
pmk8550_sleep_clk to avoid the duplicate-label error.
♻️ Duplicate comments (12)
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (2)
1591-1594: Odin2 DTS points to Odin2 Portal firmware paths.This loads Portal firmware on base Odin2. If firmware is shared, centralize/document it; otherwise switch to
qcom/sm8550/ayn/odin2/....Also applies to: 1687-1696, 1699-1704
1785-1798: Add 37.5 MHz OPP entry for level‑shifter HS mode.The driver now requests 37.5 MHz, but the OPP table only defines 100/202 MHz, so
dev_pm_opp_set_rate()will fail for level‑shifter devices. Add anopp-37500000entry with appropriate bandwidth values.Also applies to: 2129-2143
patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (1)
292-309: Regulator remains enabled on probe failure.If
htr3212_init_regs()orhtr3212_parse_dt()fails,vddstays on andsdbremains asserted. Add a cleanup path.🛠️ Proposed fix
ret = htr3212_init_regs(priv); if (ret) - return ret; + goto err_disable; ret = htr3212_parse_dt(dev, priv); if (ret) - return ret; + goto err_disable; return 0; + +err_disable: + gpiod_set_value_cansleep(priv->sdb, 0); + regulator_disable(priv->vdd); + return ret; }patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (2)
35-39: Fix typo in the 32-bit DSP read error log.Message says “32r-bit”.
🛠️ Suggested fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);
72-85: Guard odd lengths and use unaligned-safe reads for DSP writes.The 16-bit write loop assumes aligned data and even length. An odd
lenwill read past the buffer;cpu_to_be16p()is not guaranteed safe for unaligned access.🛠️ Suggested fix
+#include <linux/unaligned.h> @@ + if (len & 0x1) + return -EINVAL; + for (i = 0; i < len; i += 2) { - reg_val = cpu_to_be16p((u16 *)(data + i)); + reg_val = get_unaligned_be16(data + i); ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, (u16)reg_val);patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (5)
184-195: Skip the subcommand byte when building the MCU version string.
FRAME_POS_DATA_1holds the subcommand; copying from there includes it in the version.🛠️ Suggested fix
- if (payload_length >= 1) { + if (payload_length >= 2) { char mcu_version[MCU_VERSION_MAX_LEN] = {0}; - size_t version_length = payload_length; + size_t version_length = payload_length - 1; @@ - memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); + memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
209-237: Makeprev_statesper-device to avoid cross-instance leakage.
static prev_statesis shared across all devices. It should live instruct rsinput_driver.
281-312: RX path drops fragmented UART frames.The callback overwrites
rx_bufand validates each callback as a full frame; fragmented packets are lost and checksum failures trigger reinit. Use a persistent per-device buffer (ring/linear) and assemble complete frames before validation.
306-308: Avoid sleeping in the serdev receive callback.
rsinput_init_commands()callsmsleep()and must not run inreceive_bufcontext. Defer to a workqueue.
333-352: Return on optional GPIO errors; don’t keep ERR_PTRs.Current code only warns and continues, which can dereference error pointers in
gpiod_set_value_cansleep().🛠️ Suggested fix
drv->boot_gpio = devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); if (IS_ERR(drv->boot_gpio)) { error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n"); } @@ drv->enable_gpio = devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_HIGH); if (IS_ERR(drv->enable_gpio)) { error = PTR_ERR(drv->enable_gpio); - dev_warn(&serdev->dev, "Unable to get enable gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get enable gpio\n"); } @@ drv->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(drv->reset_gpio)) { error = PTR_ERR(drv->reset_gpio); - dev_warn(&serdev->dev, "Unable to get reset gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get reset gpio\n"); }patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (2)
123-128: Fallback to NVM ifqca_set_bdaddr()fails.Serial generation can succeed while
qca_set_bdaddr()fails; current code hard-fails instead of using the previous NVM path.
245-261: Preserve per‑pdev MACs and maintain uniqueness across pdevs.Serial-derived MACs override valid per‑pdev MACs and do not add
pdev_idx, risking duplicates on multi‑radio devices.
patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
Show resolved
Hide resolved
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 93-97: The check of AW88166_BIT_SYSINT_CHECK after calling
aw_dev_get_int_status(aw_dev, ®_val) currently logs with dev_dbg then returns
-EINVAL, which can hide a real fault; change the logging to an appropriate
error-level message (e.g., use dev_err or dev_warn_once) on aw_dev->dev so the
sysint failure is visible in normal logs while keeping the same return -EINVAL
and behavior in the function.
♻️ Duplicate comments (11)
patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch (1)
48-50: OPP table concern previously flagged.The potential issue where
dev_pm_opp_set_rate()may fail if 37.5MHz is not in the OPP table was already raised in a previous review. Please ensure the device tree patch (0018) includes a 37.5MHz OPP entry forsdhc2_opp_tableif this device usesqcom,use-level-shifter.patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (2)
34-39: Fix typo in 32-bit error message.
"32r-bit" looks unintended and makes logs harder to grep.🛠️ Proposed fix
- dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32r-bit dsp_data[0x%x] failed", + dev_err(aw_dev->dev, "read dsp_addr[0x%x] 32-bit dsp_data[0x%x] failed", (u32)dsp_addr, *dsp_data);
72-85: Guard DSP writes for odd length and unaligned data.
The 16-bit write loop readsdata + iasu16 *without alignment guarantees and can overrun whenlenis odd.🔧 Suggested fix (safe unaligned access + length check)
+#include <linux/unaligned.h>+ if (len & 0x1) + return -EINVAL; for (i = 0; i < len; i += 2) { - reg_val = cpu_to_be16p((u16 *)(data + i)); + reg_val = get_unaligned_be16(data + i); ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, (u16)reg_val);patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (2)
112-134: Keep NVM fallback ifqca_set_bdaddr()fails.
If the serial-derived BD_ADDR is generated butqca_set_bdaddr()fails, setup currently aborts instead of falling back to NVM, which can unnecessarily break BT bring-up.🐛 Proposed fix
/* Generate BD_ADDR from qcom_serial_number */ err = generate_bdaddr_from_serial(hdev, &generated_bdaddr); if (err) { bt_dev_warn(hdev, "Failed to generate BD_ADDR from serial number, falling back to NVM"); err = qca_check_bdaddr(hdev, &config); if (err) return err; } else { /* Set the generated BD_ADDR */ err = qca_set_bdaddr(hdev, &generated_bdaddr); if (err) { - bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number"); - return err; + bt_dev_err(hdev, "Failed to set the generated BD_ADDR from serial number, falling back to NVM"); + err = qca_check_bdaddr(hdev, &config); + if (err) + return err; + goto done_bdaddr; } /* Update hdev->public_addr and hdev->bdaddr */ bacpy(&hdev->public_addr, &generated_bdaddr); bacpy(&hdev->bdaddr, &generated_bdaddr); bt_dev_info(hdev, "BD_ADDR set to %pMR", &hdev->public_addr); } +done_bdaddr:
245-261: Avoid overriding valid per‑pdev MACs and ensure uniqueness when serial‑derived is used.
As written, a successful serial-derived MAC ignorespdevs_macaddr_validand doesn’t addpdev_idx, which can yield duplicate MACs on multi‑pdev devices.🐛 Proposed fix
pdev = ar->pdev; - ret = generate_macaddr_from_serial(ar, &generated_macaddr); - if (ret) { - if (ar->ab->pdevs_macaddr_valid) { - ether_addr_copy(ar->mac_addr, pdev->mac_addr); - } else { - ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); - ar->mac_addr[4] += ar->pdev_idx; - } - } else { - ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); - ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); - } + if (ar->ab->pdevs_macaddr_valid) { + ether_addr_copy(ar->mac_addr, pdev->mac_addr); + } else { + ret = generate_macaddr_from_serial(ar, &generated_macaddr); + if (!ret) { + ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); + ar->mac_addr[4] += ar->pdev_idx; + ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr); + } else { + ether_addr_copy(ar->mac_addr, ar->ab->mac_addr); + ar->mac_addr[4] += ar->pdev_idx; + } + }patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (2)
1133-1141: Avoid duplicatepmk8550_sleep_clklabel (dtc error).
pmk8550_sleep_clkis already defined in the PMK8550 base DTSI; redefining it here can trigger duplicate-label failures during dtc. Drop the local label or override via&pmk8550_sleep_clk { ... }instead.🛠️ Suggested fix (remove duplicate label)
-&pmk8550_gpios { - pmk8550_sleep_clk: sleep-clk-state { - pins = "gpio3"; - function = "func1"; - input-disable; - output-enable; - bias-disable; - power-source = <0>; - }; -}; +&pmk8550_gpios { + /* keep only local overrides here */ +};
1591-1695: Odin2 firmware paths shouldn’t point atodin2portal.The Odin2 DTS uses firmware-name entries under
qcom/sm8550/ayn/odin2portal/..., which will load Portal firmware on the base Odin2 device unless the firmware is intentionally shared. If shared, document it; otherwise switch paths toodin2.🛠️ Suggested fix
-&firmware-name = "qcom/sm8550/ayn/odin2portal/a740_zap.mbn"; +&firmware-name = "qcom/sm8550/ayn/odin2/a740_zap.mbn"; @@ -&firmware-name = "qcom/sm8550/ayn/odin2portal/adsp.mbn", - "qcom/sm8550/ayn/odin2portal/adsp_dtb.mbn"; +&firmware-name = "qcom/sm8550/ayn/odin2/adsp.mbn", + "qcom/sm8550/ayn/odin2/adsp_dtb.mbn"; @@ -&firmware-name = "qcom/sm8550/ayn/odin2portal/cdsp.mbn", - "qcom/sm8550/ayn/odin2portal/cdsp_dtb.mbn"; +&firmware-name = "qcom/sm8550/ayn/odin2/cdsp.mbn", + "qcom/sm8550/ayn/odin2/cdsp_dtb.mbn"; @@ -&firmware-name = "qcom/sm8550/ayn/odin2portal/aw883xx_acf.bin"; +&firmware-name = "qcom/sm8550/ayn/odin2/aw883xx_acf.bin";patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (4)
209-236: RX path must handle fragmented frames + per‑device state.UART data can be fragmented across callbacks; the current logic overwrites
rx_bufand validates each callback as a whole frame, causing dropped packets and spurious re-inits. Also,prev_statesisstatic, leaking state across devices. Use a persistent per-device RX buffer with framing/resync logic and moveprev_statesintostruct rsinput_driver.Also applies to: 281-311
304-309: Don’t sleep inside the serdev receive callback.
rsinput_init_commands()callsmsleep()but is invoked on checksum failure inreceive_buf, which must not sleep. Defer to a workqueue or threaded context.🛠️ Suggested fix (outline)
- if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); - goto error; - } + if (computed_checksum != received_checksum) { + schedule_work(&drv->reinit_work); + goto error; + }Additional changes:
- Add
struct work_struct reinit_work;tostruct rsinput_driver.INIT_WORK(&drv->reinit_work, rsinput_reinit_work);in probe.- Implement
rsinput_reinit_work()to callrsinput_init_commands().
333-352: Return on optional GPIO errors to avoid ERR_PTR deref.
devm_gpiod_get_optional()can returnERR_PTR. Logging and continuing leaves an error pointer that later passes theif (drv->*_gpio)check and crashes ongpiod_set_value_cansleep(). Return viadev_err_probe()for boot/enable/reset GPIOs.🛠️ Suggested fix (apply to boot/enable/reset)
- drv->boot_gpio = - devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); - if (IS_ERR(drv->boot_gpio)) { - error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); - } + drv->boot_gpio = + devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); + if (IS_ERR(drv->boot_gpio)) { + error = PTR_ERR(drv->boot_gpio); + return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n"); + }
184-195: Skip the CMD_COMMOD subcommand byte in MCU version parsing.The version string starts after the subcommand byte. Copying from
FRAME_POS_DATA_1includes the subcommand and truncates the actual version.🛠️ Suggested fix
- case DATA_COMMOD_VERSION: - if (payload_length >= 1) { + case DATA_COMMOD_VERSION: + if (payload_length >= 2) { char mcu_version[MCU_VERSION_MAX_LEN] = {0}; - size_t version_length = payload_length; + size_t version_length = payload_length - 1; if (version_length > MCU_VERSION_MAX_LEN - 1) { version_length = MCU_VERSION_MAX_LEN - 1; } - memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); + memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length); mcu_version[version_length] = '\0'; dev_info(&drv->serdev->dev, "MCU Version: %s\n", mcu_version); }
🧹 Nitpick comments (2)
patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch (1)
44-47: Minor: Inconsistent line continuation style for conditional.The
&&operator placement is inconsistent—end of line 44 vs. start of line 45. Consider consistent placement for readability.🔧 Suggested style fix
- if (timing == MMC_TIMING_SD_HS && desired_rate == 50000000 - && msm_host->uses_level_shifter) + if (timing == MMC_TIMING_SD_HS && + desired_rate == 50000000 && + msm_host->uses_level_shifter) desired_rate = LEVEL_SHIFTER_HIGH_SPEED_FREQ;patch/kernel/archive/sm8550-6.18/0008-mmc-sdhci-msm-Toggle-the-FIFO-write-clock-after-unga.patch (1)
46-72: Inconsistent access toiosstruct.Line 49 creates a local copy
ios, and line 54 usesios.enhanced_strobe, but line 55 accesseshost->mmc->ios.timingdirectly instead of usingios.timing. This is functionally equivalent but inconsistent.🔧 Suggested fix for consistency
if ((msm_host->tuning_done || ios.enhanced_strobe) && - host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { + ios.timing == MMC_TIMING_MMC_HS400) {
| aw_dev_get_int_status(aw_dev, ®_val); | ||
| if (reg_val & AW88166_BIT_SYSINT_CHECK) { | ||
| - dev_err(aw_dev->dev, "pa stop check fail:0x%04x\n", reg_val); | ||
| + dev_dbg(aw_dev->dev, "pa stop check fail:0x%04x\n", reg_val); | ||
| return -EINVAL; |
There was a problem hiding this comment.
Consider keeping error-level logging for sysint failures.
This path returns -EINVAL; downgrading to dev_dbg can hide a real fault in normal logs. Consider dev_err or dev_warn_once instead.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 93 - 97, The check of AW88166_BIT_SYSINT_CHECK after calling
aw_dev_get_int_status(aw_dev, ®_val) currently logs with dev_dbg then returns
-EINVAL, which can hide a real fault; change the logging to an appropriate
error-level message (e.g., use dev_err or dev_warn_once) on aw_dev->dev so the
sysint failure is visible in normal logs while keeping the same return -EINVAL
and behavior in the function.
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
Signed-off-by: Alex Ling <ling_kasim@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`:
- Around line 22-24: Update the Kconfig prompt for the JOYSTICK_RSINPUT option
to correct grammar and improve clarity: edit the tristate prompt string in the
JOYSTICK_RSINPUT stanza so it reads something like "UART-based gamepad driver
for AYN and Retroid Pocket devices" (or similar clear phrasing), leaving the
symbol name JOYSTICK_RSINPUT and the depends on SERIAL_DEV_BUS intact.
- Around line 209-240: The function handle_cmd_status reads up to
FRAME_POS_DATA_14 but only checks payload_length >= 6; change the guard to
verify the full required length (e.g., payload_length >= FRAME_POS_DATA_14 + 1
or a constant like 14) before accessing data[FRAME_POS_DATA_7..14], and ensure
this check encloses all reads and input_report_* calls in handle_cmd_status so
no out-of-bounds access occurs when parsing keys, axes and hat values.
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`:
- Around line 394-425: The functions icna3512_bl_update_status and
icna3512_bl_get_brightness clear dsi->mode_flags LPM but do not restore the
original flags on error paths; save the original flags into a local variable
(e.g., orig_mode_flags) before modifying dsi->mode_flags, and restore
dsi->mode_flags = orig_mode_flags before any early return (including on ret < 0)
and at the end of each function so the DSI mode_flags are preserved regardless
of I/O success or failure.
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`:
- Line 218: Replace the incorrect Bluetooth-format specifier with the standard
MAC formatter in the ath12k logging call: change the usage in the ath12k_info
call that prints macaddr (currently using "%pMR") to use "%pM" so the MAC
(macaddr) is logged in standard network byte order; update the
ath12k_info(ar->ab, "Generated MAC_ADDR: %pMR", macaddr) invocation accordingly.
- Around line 154-156: Remove the unused static struct declaration by deleting
the static_macaddr symbol and its initializer (the entire static const struct
macaddr_t static_macaddr = { .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } };
block); ensure there are no remaining references to static_macaddr elsewhere
(similar to the removal of static_bdaddr in btqca.c) and run a build/compile to
confirm no undefined-symbol errors remain.
In
`@patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch`:
- Around line 50-87: The AW88166_DSP_I2C_WRITES branch is never defined so the
else path with unsafe unaligned access (cpu_to_be16p((u16 *)(data + i))) and no
handling of odd len becomes active; fix by either defining
AW88166_DSP_I2C_WRITES where intended so the regmap_raw_write path (using
AW88166_MAX_RAM_WRITE_BYTE_SIZE and regmap_raw_write) is compiled, or modify the
else branch in the functions using AW88166_DSPMADD_REG/AW88166_DSPMDAT_REG to
handle alignment and odd lengths safely (validate len, use get_unaligned_be16 or
memcpy into a u16 before cpu_to_be16 or write the trailing byte separately) and
ensure mutex_lock/unlock paths remain correct.
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`:
- Around line 1732-1736: Fix the typo in the comment block describing DLL HSR
settings: remove the stray "0" from the sentence mentioning DLL_CONFIG_2 so it
reads clearly (e.g., "Note that the DLL_CONFIG_2 value is not passed from the
device tree, but it is calculated in the driver.")—update the comment near the
DLL HSR settings / DLL_CONFIG_2 mention.
♻️ Duplicate comments (15)
patch/kernel/archive/sm8550-6.18/0007-mmc-sdhci-msm-Limit-HS-mode-frequency-to-37.5MHz.patch (1)
39-48: Ensure 37.5 MHz exists in the SDHCI OPP table.
If the OPP table doesn’t include 37.5 MHz,dev_pm_opp_set_rate()will fail for HS mode on level‑shifter devices. This concern was already raised earlier.patch/kernel/archive/sm8550-6.18/0005-ASoC-qcom-sc8280xp-Add-support-for-Primary-I2S.patch (1)
34-55: Startup error-handling gaps were already flagged.
Same concern as prior review (missing error checks forclk_prepare_enable()/snd_soc_dai_set_fmt()and cleanup onqcom_snd_sdw_startup()failure).patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
1595-1700: Firmware paths referenceodin2portalinodin2device tree.This was flagged in a previous review: the
qcs8550-ayn-odin2.dtsfile uses firmware paths pointing toqcom/sm8550/ayn/odin2portal/for:
- GPU zap-shader (line 1597)
- ADSP firmware (lines 1683-1684)
- CDSP firmware (lines 1689-1690)
- Speaker amplifier firmware (lines 1695, 1699)
If both devices intentionally share the same firmware, consider adding a comment documenting this; otherwise, create separate firmware paths for each variant.
patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (2)
34-40: Fix typo in error log ("32r-bit" should be "32-bit").The 32-bit case error message has a typo that should be corrected for consistency with the 16-bit case.
91-99: Consider keeping error-level logging for sysint failures.Downgrading to
dev_dbgwhile returning-EINVALcan hide real faults in normal logs.patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch (1)
256-261: Use%pMformat and verify per-pdev uniqueness is handled.Two observations:
Line 260 uses
%pMRwhich is for Bluetooth addresses—use%pMfor MAC addresses.The success path applies the same serial-derived MAC to all pdevs without adding
ar->pdev_idxoffset. This was flagged previously and marked as addressed. Please confirm the uniqueness fix was applied, as multi-radio devices would otherwise get duplicate MAC addresses.🔧 Proposed fix for format specifier
} else { ath12k_reverse_mac(ar->mac_addr, generated_macaddr.b); - ath12k_info(ab, "MAC_ADDR set to %pMR", generated_macaddr.b); + ar->mac_addr[4] += ar->pdev_idx; + ath12k_info(ab, "MAC_ADDR set to %pM", ar->mac_addr); }patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (5)
184-195: Skip the CMD_COMMOD subcommand byte when parsing MCU version.
The version string should start after the subcommand; currently it includes it.🛠️ Suggested fix
- if (payload_length >= 1) { + if (payload_length >= 2) { char mcu_version[MCU_VERSION_MAX_LEN] = {0}; - size_t version_length = payload_length; + size_t version_length = payload_length - 1; if (version_length > MCU_VERSION_MAX_LEN - 1) { version_length = MCU_VERSION_MAX_LEN - 1; } - memcpy(mcu_version, &data[FRAME_POS_DATA_1], version_length); + memcpy(mcu_version, &data[FRAME_POS_DATA_2], version_length);
210-237: Makeprev_statesper-device, notstatic.
staticstate leaks across device instances. Track it instruct rsinput_driverinstead.🛠️ Suggested change
struct rsinput_driver { struct serdev_device *serdev; struct input_dev *input; struct gpio_desc *boot_gpio; struct gpio_desc *enable_gpio; struct gpio_desc *reset_gpio; uint8_t rx_buf[256]; uint8_t sequence_number; + unsigned long prev_states; }; static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { if (payload_length >= 14) { - static unsigned long prev_states; unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); unsigned long current_states = keys, changes; int i; - bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); + bitmap_xor(&changes, ¤t_states, &drv->prev_states, ARRAY_SIZE(keymap)); ... - prev_states = keys; + drv->prev_states = keys;
281-312: RX path assumes each callback is a full frame.
UART RX commonly fragments frames across callbacks. The current logic overwritesrx_bufper callback and validates the whole callback as one frame, losing fragments and causing spurious errors. Consider a persistent buffer with header scan and frame assembly.
306-308: Don’t sleep inreceive_bufcontext.
rsinput_init_commands()usesmsleep(100)and is called from the RX callback on checksum failure. Defer this to a workqueue or other process context.🛠️ Sketch of a safer pattern
- if (computed_checksum != received_checksum) { - rsinput_init_commands(drv); - goto error; - } + if (computed_checksum != received_checksum) { + schedule_work(&drv->reinit_work); + goto error; + }
333-352: Handle optional GPIO errors by returning, not continuing.
devm_gpiod_get_optional()can return an error pointer. These are later treated as valid and dereferenced viagpiod_set_value_cansleep(). Return on error (usingdev_err_probe) to avoid crashes and to handle-EPROBE_DEFER.Apply the same pattern to `enable` and `reset`.🛠️ Safer error handling
drv->boot_gpio = devm_gpiod_get_optional(&serdev->dev, "boot", GPIOD_OUT_HIGH); if (IS_ERR(drv->boot_gpio)) { error = PTR_ERR(drv->boot_gpio); - dev_warn(&serdev->dev, "Unable to get boot gpio: %d\n", error); + return dev_err_probe(&serdev->dev, error, "Unable to get boot gpio\n"); }patch/kernel/archive/sm8550-6.18/0004-leds-Add-driver-for-HEROIC-HTR3212.patch (2)
292-309: Regulator cleanup is still missing on probe failure.
This matches a previously reported issue; ensure the regulator is disabled on error paths to avoid leaks.
348-348: MODULE_AUTHOR string is still malformed.
Missing the closing>and quote.🔧 Proposed fix
-MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in"); +MODULE_AUTHOR("Teguh Sobirin <teguh@sobir.in>");patch/kernel/archive/sm8550-6.18/0010-drm-panel-Add-WIP-panel-driver-for-AYN-Odin-2.patch (1)
99-149: Restoredsi->mode_flagson failure paths intd4328_on()/td4328_off().
If any DCS command fails, the LPM toggle remains applied and can leak into subsequent DSI traffic. Restore the original flags before returning on error.Proposed fix (restore flags on error)
static int td4328_on(struct td4328 *ctx) { struct mipi_dsi_device *dsi = ctx->dsi; struct device *dev = &dsi->dev; + unsigned long mode_flags = dsi->mode_flags; int ret; dsi->mode_flags |= MIPI_DSI_MODE_LPM; @@ ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret < 0) { dev_err(dev, "Failed to exit sleep mode: %d\n", ret); + dsi->mode_flags = mode_flags; return ret; } @@ ret = mipi_dsi_dcs_set_display_on(dsi); if (ret < 0) { dev_err(dev, "Failed to set display on: %d\n", ret); + dsi->mode_flags = mode_flags; return ret; } @@ static int td4328_off(struct td4328 *ctx) { struct mipi_dsi_device *dsi = ctx->dsi; struct device *dev = &dsi->dev; + unsigned long mode_flags = dsi->mode_flags; int ret; dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; @@ ret = mipi_dsi_dcs_set_display_off(dsi); if (ret < 0) { dev_err(dev, "Failed to set display off: %d\n", ret); + dsi->mode_flags = mode_flags; return ret; } @@ ret = mipi_dsi_dcs_enter_sleep_mode(dsi); if (ret < 0) { dev_err(dev, "Failed to enter sleep mode: %d\n", ret); + dsi->mode_flags = mode_flags; return ret; }patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch (1)
286-299: Clean up power/reset when PPS or compression setup fails.
Failure returns skip regulator disable/reset, leaving hardware in an inconsistent state.Proposed fix (common error cleanup)
ret = mipi_dsi_picture_parameter_set(pinfo->dsi, &pps); if (ret < 0) { dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); - return ret; + goto err_disable; } /* Not required, ICNA3512 has DSC always enabled. */ ret = mipi_dsi_compression_mode(pinfo->dsi, true); if (ret < 0) { dev_err(panel->dev, "failed to enable compression mode: %d\n", ret); - return ret; + goto err_disable; } return 0; + +err_disable: + gpiod_set_value_cansleep(pinfo->reset_gpio, 1); + regulator_bulk_disable(ARRAY_SIZE(pinfo->supplies), pinfo->supplies); + return ret;
🧹 Nitpick comments (4)
patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch (1)
244-358: Consider removing commented-out thermal trip points and cooling maps.The thermal zone section contains significant dead code (commented-out trip points at lines 247-263, 307-317, and cooling maps at lines 349-356). If these are intentionally disabled configurations for future use, consider documenting why they're preserved; otherwise, remove them to improve maintainability.
patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch (1)
370-377: Minor style: missing space before{.Linux kernel coding style requires a space before the opening brace.
🔧 Proposed fix
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK){ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch (1)
128-155: Guard against frame buffer overflow inrsinput_send_command().
frameis fixed at 256 bytes; add a hard cap to avoid accidental overflow if future callers pass a larger payload.🛡️ Proposed guard
static int rsinput_send_command(struct rsinput_driver *drv, uint8_t cmd, const uint8_t *data, size_t len) { uint8_t frame[256]; uint8_t checksum = 0; size_t frame_len = 0; + if (len > sizeof(frame) - (FRAME_POS_DATA_1 + 1)) + return -EINVAL;patch/kernel/archive/sm8550-6.18/0017-pwm-Add-SI-EN-SN3112-PWM-support.patch (1)
85-86: Useu8instead ofuint8_tfor kernel coding style consistency.In Linux kernel 6.18,
uint8_tis defined ininclude/linux/types.has a typedef ofu8, so there is no compilation risk. However, kernel coding style recommends usingu8for kernel-internal types rather thanuint8_t. Update lines 85–86 and the function parameter at line 144–145.
| +config JOYSTICK_RSINPUT | ||
| + tristate "UART Based gamepad driver that found in AYN and Retroid Pocket products" | ||
| + depends on SERIAL_DEV_BUS |
There was a problem hiding this comment.
Fix the Kconfig prompt grammar.
The current prompt reads awkwardly for a user-facing option.
✏️ Suggested wording
-config JOYSTICK_RSINPUT
- tristate "UART Based gamepad driver that found in AYN and Retroid Pocket products"
+config JOYSTICK_RSINPUT
+ tristate "UART-based gamepad driver found in AYN and Retroid Pocket devices"🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 22 - 24, Update the Kconfig prompt for the JOYSTICK_RSINPUT option
to correct grammar and improve clarity: edit the tristate prompt string in the
JOYSTICK_RSINPUT stanza so it reads something like "UART-based gamepad driver
for AYN and Retroid Pocket devices" (or similar clear phrasing), leaving the
symbol name JOYSTICK_RSINPUT and the depends on SERIAL_DEV_BUS intact.
| +static void handle_cmd_status(struct rsinput_driver *drv, const uint8_t *data, size_t payload_length) { | ||
| + if (payload_length >= 6) { | ||
| + static unsigned long prev_states; | ||
| + unsigned long keys = data[FRAME_POS_DATA_1] | (data[FRAME_POS_DATA_2] << 8); | ||
| + unsigned long current_states = keys, changes; | ||
| + int i; | ||
| + | ||
| + bitmap_xor(&changes, ¤t_states, &prev_states, ARRAY_SIZE(keymap)); | ||
| + | ||
| + for_each_set_bit(i, &changes, ARRAY_SIZE(keymap)) { | ||
| + input_report_key(drv->input, keymap[i], (current_states & BIT(i))); | ||
| + } | ||
| + | ||
| + input_report_abs(drv->input, ABS_HAT2X, | ||
| + 0x650 - (data[FRAME_POS_DATA_3] | (data[FRAME_POS_DATA_4] << 8))); | ||
| + input_report_abs(drv->input, ABS_HAT2Y, | ||
| + 0x650 - (data[FRAME_POS_DATA_5] | (data[FRAME_POS_DATA_6] << 8))); | ||
| + input_report_abs(drv->input, ABS_X, | ||
| + -(int16_t)(data[FRAME_POS_DATA_7] | (data[FRAME_POS_DATA_8] << 8))); | ||
| + input_report_abs(drv->input, ABS_Y, | ||
| + -(int16_t)(data[FRAME_POS_DATA_9] | (data[FRAME_POS_DATA_10] << 8))); | ||
| + input_report_abs(drv->input, ABS_RX, | ||
| + -(int16_t)(data[FRAME_POS_DATA_11] | (data[FRAME_POS_DATA_12] << 8))); | ||
| + input_report_abs(drv->input, ABS_RY, | ||
| + -(int16_t)(data[FRAME_POS_DATA_13] | (data[FRAME_POS_DATA_14] << 8))); | ||
| + | ||
| + input_sync(drv->input); | ||
| + prev_states = keys; | ||
| + | ||
| + } else { | ||
| + dev_warn(&drv->serdev->dev, "Invalid CMD_STATUS response length\n"); | ||
| + } |
There was a problem hiding this comment.
Validate CMD_STATUS payload length before reading 14 bytes.
The handler reads through FRAME_POS_DATA_14, but only checks payload_length >= 6. This can read beyond the frame if the MCU sends a shorter payload.
✅ Safer length check
- if (payload_length >= 6) {
+ if (payload_length >= 14) {
...
- } else {
- dev_warn(&drv->serdev->dev, "Invalid CMD_STATUS response length\n");
+ } else {
+ dev_warn(&drv->serdev->dev, "Invalid CMD_STATUS response length (need >= 14)\n");
}🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0002-input-Add-driver-for-RSInput-Gamepad.patch`
around lines 209 - 240, The function handle_cmd_status reads up to
FRAME_POS_DATA_14 but only checks payload_length >= 6; change the guard to
verify the full required length (e.g., payload_length >= FRAME_POS_DATA_14 + 1
or a constant like 14) before accessing data[FRAME_POS_DATA_7..14], and ensure
this check encloses all reads and input_report_* calls in handle_cmd_status so
no out-of-bounds access occurs when parsing keys, axes and hat values.
| +static int icna3512_bl_update_status(struct backlight_device *bl) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = bl_get_data(bl); | ||
| + u16 brightness = backlight_get_brightness(bl); | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; | ||
| + | ||
| + ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness); | ||
| + if (ret < 0) | ||
| + return ret; | ||
| + | ||
| + dsi->mode_flags |= MIPI_DSI_MODE_LPM; | ||
| + | ||
| + return 0; | ||
| +} | ||
| + | ||
| +static int icna3512_bl_get_brightness(struct backlight_device *bl) | ||
| +{ | ||
| + struct mipi_dsi_device *dsi = bl_get_data(bl); | ||
| + u16 brightness; | ||
| + int ret; | ||
| + | ||
| + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; | ||
| + | ||
| + ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness); | ||
| + if (ret < 0) | ||
| + return ret; | ||
| + | ||
| + dsi->mode_flags |= MIPI_DSI_MODE_LPM; | ||
| + | ||
| + return brightness; |
There was a problem hiding this comment.
Restore mode_flags if brightness I/O fails.
On error, LPM stays cleared and can alter subsequent DSI traffic. Preserve and restore the original flags.
Proposed fix (save/restore flags)
static int icna3512_bl_update_status(struct backlight_device *bl)
{
struct mipi_dsi_device *dsi = bl_get_data(bl);
u16 brightness = backlight_get_brightness(bl);
+ unsigned long mode_flags = dsi->mode_flags;
int ret;
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
- if (ret < 0)
- return ret;
-
- dsi->mode_flags |= MIPI_DSI_MODE_LPM;
-
- return 0;
+ dsi->mode_flags = mode_flags;
+ return ret;
}
@@
static int icna3512_bl_get_brightness(struct backlight_device *bl)
{
struct mipi_dsi_device *dsi = bl_get_data(bl);
u16 brightness;
+ unsigned long mode_flags = dsi->mode_flags;
int ret;
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
- if (ret < 0)
- return ret;
-
- dsi->mode_flags |= MIPI_DSI_MODE_LPM;
-
- return brightness;
+ dsi->mode_flags = mode_flags;
+ if (ret < 0)
+ return ret;
+ return brightness;
}🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0003-drm-panel-Add-panel-driver-for-Chipone-ICNA3512-base.patch`
around lines 394 - 425, The functions icna3512_bl_update_status and
icna3512_bl_get_brightness clear dsi->mode_flags LPM but do not restore the
original flags on error paths; save the original flags into a local variable
(e.g., orig_mode_flags) before modifying dsi->mode_flags, and restore
dsi->mode_flags = orig_mode_flags before any early return (including on ret < 0)
and at the end of each function so the DSI mode_flags are preserved regardless
of I/O success or failure.
| +static const struct macaddr_t static_macaddr = { | ||
| + .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } | ||
| +}; |
There was a problem hiding this comment.
Remove unused static_macaddr declaration.
Similar to the previously removed static_bdaddr in btqca.c, this static structure is defined but never referenced in the code.
🧹 Proposed removal
-/* Define a static, predefined MAC_ADDR structure */
-static const struct macaddr_t static_macaddr = {
- .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 }
-};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +static const struct macaddr_t static_macaddr = { | |
| + .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } | |
| +}; |
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
around lines 154 - 156, Remove the unused static struct declaration by deleting
the static_macaddr symbol and its initializer (the entire static const struct
macaddr_t static_macaddr = { .b = { 0x00, 0x03, 0x7F, 0x11, 0x22, 0x33 } };
block); ensure there are no remaining references to static_macaddr elsewhere
(similar to the removal of static_bdaddr in btqca.c) and run a build/compile to
confirm no undefined-symbol errors remain.
| + macaddr->b[2 - i] = byte_val; // Assign to bytes 2,1,0 | ||
| + } | ||
| + | ||
| + ath12k_info(ar->ab, "Generated MAC_ADDR: %pMR", macaddr); |
There was a problem hiding this comment.
Use %pM instead of %pMR for MAC address logging.
%pMR is specifically for bdaddr_t * (Bluetooth addresses in reversed byte order). For standard MAC addresses, use %pM which prints in network byte order (standard colon-separated format).
🔧 Proposed fix
- ath12k_info(ar->ab, "Generated MAC_ADDR: %pMR", macaddr);
+ ath12k_info(ar->ab, "Generated MAC_ADDR: %pM", macaddr->b);🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0015-drivers-use-soc-serial-for-wifi-and-bluetooth.patch`
at line 218, Replace the incorrect Bluetooth-format specifier with the standard
MAC formatter in the ath12k logging call: change the usage in the ath12k_info
call that prints macaddr (currently using "%pMR") to use "%pM" so the MAC
(macaddr) is logged in standard network byte order; update the
ath12k_info(ar->ab, "Generated MAC_ADDR: %pMR", macaddr) invocation accordingly.
| +#ifdef AW88166_DSP_I2C_WRITES | ||
| + u32 tmp_len; | ||
| + | ||
| mutex_lock(&aw_dev->dsp_lock); | ||
| ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | ||
| if (ret) | ||
| goto error_operation; | ||
|
|
||
| for (i = 0; i < len; i += AW88166_MAX_RAM_WRITE_BYTE_SIZE) { | ||
| - tmp_len = min(len - i, AW88166_MAX_RAM_WRITE_BYTE_SIZE); | ||
| + if ((len - i) < AW88166_MAX_RAM_WRITE_BYTE_SIZE) | ||
| + tmp_len = len - i; | ||
| + else | ||
| + tmp_len = AW88166_MAX_RAM_WRITE_BYTE_SIZE; | ||
| + | ||
| ret = regmap_raw_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | ||
| &data[i], tmp_len); | ||
| if (ret) | ||
| goto error_operation; | ||
| } | ||
| mutex_unlock(&aw_dev->dsp_lock); | ||
| +#else | ||
| + __be16 reg_val; | ||
| + | ||
| + mutex_lock(&aw_dev->dsp_lock); | ||
| + /* i2c write */ | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMADD_REG, base); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + for (i = 0; i < len; i += 2) { | ||
| + reg_val = cpu_to_be16p((u16 *)(data + i)); | ||
| + ret = regmap_write(aw_dev->regmap, AW88166_DSPMDAT_REG, | ||
| + (u16)reg_val); | ||
| + if (ret) | ||
| + goto error_operation; | ||
| + } | ||
| + mutex_unlock(&aw_dev->dsp_lock); | ||
| +#endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if AW88166_DSP_I2C_WRITES is defined anywhere in the sm8550-6.18 patches or kernel headers
echo "=== Searching in sm8550-6.18 patches ==="
rg -n "AW88166_DSP_I2C_WRITES" patch/kernel/archive/sm8550-6.18/
echo ""
echo "=== Searching entire patch directory for this macro ==="
rg -n "AW88166_DSP_I2C_WRITES" patch/kernel/
echo ""
echo "=== Checking the specific patch file for context ==="
grep -n "AW88166_DSP_I2C_WRITES\|define" patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch | head -20Repository: armbian/build
Length of output: 835
🏁 Script executed:
#!/bin/bash
# Look for where AW88166_DSP_I2C_WRITES might be defined in the driver headers
echo "=== Searching for any `#define` of AW88166_DSP_I2C_WRITES in the entire repo ==="
rg -n "define.*AW88166_DSP_I2C_WRITES"
echo ""
echo "=== Looking at the full patch to see if it defines this macro ==="
head -100 patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch | grep -n "define\|AW88166_DSP_I2C_WRITES"
echo ""
echo "=== Check what files are in the sm8550-6.18 archive directory ==="
ls -la patch/kernel/archive/sm8550-6.18/ | grep -i aw88Repository: armbian/build
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Check if this macro is intended to come from kernel headers by looking at context
echo "=== Read more of the patch to understand intent ==="
head -60 patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch | tail -20
echo ""
echo "=== Check the patch subject and description ==="
head -20 patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch
echo ""
echo "=== Look for any kernel version info that might define this macro ==="
rg -n "AW88166" patch/kernel/archive/sm8550-6.18/ -A 2 -B 2 | head -40Repository: armbian/build
Length of output: 6007
🏁 Script executed:
#!/bin/bash
# Check if AW88166_DSP_I2C_WRITES is defined in the 6.12 version to understand intent
echo "=== Check sm8550-6.12 patch for the same function ==="
grep -A 50 "aw_dev_dsp_update_container" patch/kernel/archive/sm8550-6.12/0047_ASoC--codecs--aw88166--AYN-Odin2-Specific-modifica.patch | head -60
echo ""
echo "=== Check if macro is defined in 6.12 kernel version ==="
rg -n "define.*AW88166_DSP_I2C_WRITES" patch/kernel/archive/sm8550-6.12/Repository: armbian/build
Length of output: 1756
🏁 Script executed:
#!/bin/bash
# Check if the macro might be conditionally defined based on kernel version
echo "=== Look for CONFIG or version-based definitions ==="
rg -n "AW88166_DSP_I2C_WRITES\|define AW88166\|ifdef.*CONFIG" patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch | head -30
echo ""
echo "=== Check if this is a known pattern - look at the patch more broadly ==="
wc -l patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch
echo ""
echo "=== Search for any comments explaining the conditional ==="
grep -B 5 -A 5 "AW88166_DSP_I2C_WRITES" patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patchRepository: armbian/build
Length of output: 587
AW88166_DSP_I2C_WRITES is undefined; the else branch (lines 71-87) with unaligned access concerns will be compiled.
This macro is not defined anywhere in the repository, meaning the #ifdef AW88166_DSP_I2C_WRITES check will always be false at compile time. The #else branch (lines 71-87) containing the I2C fallback code will be the active path. This branch has unaligned pointer access with cpu_to_be16p((u16 *)(data + i)) at line 80 and lacks length validation for odd-length buffers.
If the macro is intentionally undefined as a fallback, consider either: (1) fixing the alignment issue in the active code path, or (2) defining the macro conditionally so the safer regmap_raw_write path is used instead.
🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0016-HACK-Revert-back-to-6.12-aw88166-driver.patch`
around lines 50 - 87, The AW88166_DSP_I2C_WRITES branch is never defined so the
else path with unsafe unaligned access (cpu_to_be16p((u16 *)(data + i))) and no
handling of odd len becomes active; fix by either defining
AW88166_DSP_I2C_WRITES where intended so the regmap_raw_write path (using
AW88166_MAX_RAM_WRITE_BYTE_SIZE and regmap_raw_write) is compiled, or modify the
else branch in the functions using AW88166_DSPMADD_REG/AW88166_DSPMDAT_REG to
handle alignment and odd lengths safely (validate len, use get_unaligned_be16 or
memcpy into a u16 before cpu_to_be16 or write the trailing byte separately) and
ensure mutex_lock/unlock paths remain correct.
| + /* | ||
| + * DLL HSR settings. Refer go/hsr - <Target> DLL settings. | ||
| + * Note that the DLL_CONFIG_2 value is not passed from the | ||
| + * device tree, 0 but it is calculated in the driver. | ||
| + */ |
There was a problem hiding this comment.
Minor comment typo.
The comment has an extra "0" that creates a grammatically incorrect sentence.
📝 Proposed fix
/*
* DLL HSR settings. Refer go/hsr - <Target> DLL settings.
* Note that the DLL_CONFIG_2 value is not passed from the
- * device tree, 0 but it is calculated in the driver.
+ * device tree, but it is calculated in the driver.
*/🤖 Prompt for AI Agents
In
`@patch/kernel/archive/sm8550-6.18/0018-dts-arm64-qcom-Added-Ayn-Odin2-device-support.patch`
around lines 1732 - 1736, Fix the typo in the comment block describing DLL HSR
settings: remove the stray "0" from the sentence mentioning DLL_CONFIG_2 so it
reads clearly (e.g., "Note that the DLL_CONFIG_2 value is not passed from the
device tree, but it is calculated in the driver.")—update the comment near the
DLL HSR settings / DLL_CONFIG_2 mention.
|
✅ This PR has been reviewed and approved — all set for merge! |
github-actions
bot
removed
the
Ready to merge
|
https://paste.armbian.com/exapuxadiy |
I think the problem is here. ayn-odin2 does not support forky yet.
In config/boards/ayn-odin2.csc BTW, this PR did not change that part :) |
|
I think i will simply change community to trixie. |
2 participants
Description
This PR includes initial support for running Linux kernel 6.18 on Ayn odin2 device
Documentation summary for feature / change
Basically this is a porting of existing 6.12 patches to 6.18. Dropped some ones which are included in 6.18 already.
Verified following functions are working:
Not working:
How Has This Been Tested?
Summary by CodeRabbit
New Features
Hardware Support
Bug Fixes
System Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
[1] https://github.com/quic/v4l-video-test-app